Skip to content

Commit 165b764

Browse files
committed
Bug 2004779 - Avoid parsing JSON for the packaged data if it's outdated, by storing the timestamp in a separate file.
1 parent caf03ed commit 165b764

File tree

10 files changed

+65
-12
lines changed

10 files changed

+65
-12
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: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,11 @@ impl<C: ApiClient> RemoteSettingsClient<C> {
219219
.and_then(|data| serde_json::from_str(data).ok())
220220
}
221221

222+
fn load_packaged_timestamp(&self) -> Option<u64> {
223+
// Using the macro generated `get_packaged_timestamp` in macros.rs
224+
Self::get_packaged_timestamp(&self.collection_name)
225+
}
226+
222227
fn load_packaged_attachment(&self, filename: &str) -> Option<(&'static [u8], &'static str)> {
223228
// Using the macro generated `get_packaged_attachment` in macros.rs
224229
Self::get_packaged_attachment(&self.collection_name, filename)
@@ -241,30 +246,47 @@ impl<C: ApiClient> RemoteSettingsClient<C> {
241246
.collect()
242247
}
243248

249+
/// Returns the parsed packaged data, but only if it's newer than the data we have
250+
/// in storage. This avoids parsing the packaged data if we won't use it.
251+
///
252+
/// Does not mutate `storage`.
253+
fn get_packaged_data_if_newer(
254+
&self,
255+
storage: &mut Storage,
256+
collection_url: &str,
257+
) -> Result<Option<CollectionData>> {
258+
let packaged_timestamp = match self.load_packaged_timestamp() {
259+
Some(ts) => ts,
260+
None => return Ok(None),
261+
};
262+
match storage.get_last_modified_timestamp(collection_url)? {
263+
Some(storage_timestamp) if storage_timestamp >= packaged_timestamp => {
264+
// Data in storage is newer, don't parse packaged data.
265+
Ok(None)
266+
}
267+
_ => {
268+
// Packaged data is newer, parse packaged JSON
269+
Ok(self.load_packaged_data())
270+
}
271+
}
272+
}
273+
244274
/// Get the current set of records.
245275
///
246276
/// If records are not present in storage this will normally return None. Use `sync_if_empty =
247277
/// true` to change this behavior and perform a network request in this case.
248278
pub fn get_records(&self, sync_if_empty: bool) -> Result<Option<Vec<RemoteSettingsRecord>>> {
249279
let mut inner = self.inner.lock();
250280
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-
};
257281

258282
// Case 1: The packaged data is more recent than the cache
259283
//
260284
// This happens when there's no cached data or when we get new packaged data because of a
261285
// 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 {
286+
if inner.api_client.is_prod_server()? {
287+
if let Some(packaged_data) =
288+
self.get_packaged_data_if_newer(&mut inner.storage, &collection_url)?
289+
{
268290
// Remove previously cached data (packaged data does not have tombstones like diff responses do).
269291
inner.storage.empty()?;
270292
// 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)