Skip to content

Commit 6087f7c

Browse files
authored
Bug 2004779 - Avoid parsing JSON for the packaged data if it's outdated, by storing the timestamp in a separate file. (#7121)
1 parent 8b86f87 commit 6087f7c

File tree

10 files changed

+66
-14
lines changed

10 files changed

+66
-14
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
1600363002708
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
1763049497744
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
1764082724032
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
1757010621729
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
1755604678567
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
1761148716130
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
1749069444811

components/remote_settings/src/client.rs

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -213,10 +213,17 @@ impl<C: ApiClient> RemoteSettingsClient<C> {
213213
&self.collection_name
214214
}
215215

216+
fn load_packaged_timestamp(&self) -> Option<u64> {
217+
// Using the macro generated `get_packaged_timestamp` in macros.rs
218+
Self::get_packaged_timestamp(&self.collection_name)
219+
}
220+
216221
fn load_packaged_data(&self) -> Option<CollectionData> {
217222
// Using the macro generated `get_packaged_data` in macros.rs
218-
Self::get_packaged_data(&self.collection_name)
219-
.and_then(|data| serde_json::from_str(data).ok())
223+
let str_data = Self::get_packaged_data(&self.collection_name)?;
224+
let data: CollectionData = serde_json::from_str(str_data).ok()?;
225+
debug_assert_eq!(data.timestamp, self.load_packaged_timestamp().unwrap());
226+
Some(data)
220227
}
221228

222229
fn load_packaged_attachment(&self, filename: &str) -> Option<(&'static [u8], &'static str)> {
@@ -241,30 +248,44 @@ impl<C: ApiClient> RemoteSettingsClient<C> {
241248
.collect()
242249
}
243250

251+
/// Returns the parsed packaged data, but only if it's newer than the data we have
252+
/// in storage. This avoids parsing the packaged data if we won't use it.
253+
fn get_packaged_data_if_newer(
254+
&self,
255+
storage: &mut Storage,
256+
collection_url: &str,
257+
) -> Result<Option<CollectionData>> {
258+
let packaged_ts = self.load_packaged_timestamp();
259+
let storage_ts = storage.get_last_modified_timestamp(collection_url)?;
260+
let packaged_is_newer = match (packaged_ts, storage_ts) {
261+
(Some(packaged_ts), Some(storage_ts)) => packaged_ts > storage_ts,
262+
(Some(_), None) => true, // no storage data
263+
(None, _) => false, // no packaged data
264+
};
265+
266+
if packaged_is_newer {
267+
Ok(self.load_packaged_data())
268+
} else {
269+
Ok(None)
270+
}
271+
}
272+
244273
/// Get the current set of records.
245274
///
246275
/// If records are not present in storage this will normally return None. Use `sync_if_empty =
247276
/// true` to change this behavior and perform a network request in this case.
248277
pub fn get_records(&self, sync_if_empty: bool) -> Result<Option<Vec<RemoteSettingsRecord>>> {
249278
let mut inner = self.inner.lock();
250279
let collection_url = inner.api_client.collection_url();
251-
let is_prod = inner.api_client.is_prod_server()?;
252-
let packaged_data = if is_prod {
253-
self.load_packaged_data()
254-
} else {
255-
None
256-
};
257280

258281
// Case 1: The packaged data is more recent than the cache
259282
//
260283
// This happens when there's no cached data or when we get new packaged data because of a
261284
// product update
262-
if let Some(packaged_data) = packaged_data {
263-
let cached_timestamp = inner
264-
.storage
265-
.get_last_modified_timestamp(&collection_url)?
266-
.unwrap_or(0);
267-
if packaged_data.timestamp > cached_timestamp {
285+
if inner.api_client.is_prod_server()? {
286+
if let Some(packaged_data) =
287+
self.get_packaged_data_if_newer(&mut inner.storage, &collection_url)?
288+
{
268289
// Remove previously cached data (packaged data does not have tombstones like diff responses do).
269290
inner.storage.empty()?;
270291
// Insert new packaged data.

components/remote_settings/src/macros.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,26 @@ macro_rules! packaged_collections {
1818
_ => None,
1919
}
2020
}
21+
22+
/// Get just the timestamp, which is stored separately. This allows
23+
/// checking which data is newer without paying the cost of parsing
24+
/// the full packaged JSON.
25+
fn get_packaged_timestamp(collection_name: &str) -> Option<u64> {
26+
match collection_name {
27+
$($collection => {
28+
let timestamp_str = include_str!(concat!(
29+
env!("CARGO_MANIFEST_DIR"),
30+
"/dumps/",
31+
$bucket,
32+
"/",
33+
$collection,
34+
".timestamp"
35+
));
36+
timestamp_str.trim().parse().ok()
37+
}),*
38+
_ => None,
39+
}
40+
}
2141
};
2242
}
2343

examples/remote-settings-cli/src/dump/client.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,10 @@ impl CollectionDownloader {
432432
}
433433
std::fs::write(&dumps_path, serde_json::to_string_pretty(&data)?)?;
434434

435+
// Write timestamp file for fast timestamp checking without JSON parsing
436+
let timestamp_path = dumps_path.with_extension("timestamp");
437+
std::fs::write(&timestamp_path, data.timestamp.to_string())?;
438+
435439
// Count attachments needing updates
436440
for record in &data.data {
437441
if let Some(attachment) = record.get("attachment") {

0 commit comments

Comments
 (0)