Closed Bug 768074 Opened 8 years ago Closed 4 years ago

Update Snappy library to revision >= r59 to get ARM optimizations

Categories

(Core :: Storage: IndexedDB, defect, P3)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: cpeterson, Assigned: janv)

References

(Blocks 1 open bug, )

Details

(Keywords: perf, Whiteboard: [c= p= s= u=])

Attachments

(3 files, 1 obsolete file)

Snappy r59 enables the use of unaligned loads and stores for ARM-based architectures where they are available (ARMv7 and higher). This gives a significant
speed boost on ARM, both for compression and decompression. It should not affect x86 at all.

https://code.google.com/p/snappy/source/detail?r=59
Fennec's and B2G's IndexedDB performance may benefit from new ARM optimizations in Snappy r59.
tracking-fennec: --- → ?
Chris, please measure the performance difference and renom when you have data.
tracking-fennec: ? → ---
Dietrich, B2G may be interested in this possible performance improvement for IndexedDB. We compress IndexedDB data using Google's Snappy compression library, but our tree's snapshot is missing some recent Snappy optimizations for ARMv7. Some of their microbenchmarks improved ~20-30%.
Flags: needinfo?(dietrich)
Mlee, can you add this to the backlog? It might be worth having someone from the perf team spend some time seeing what the on-device benefits are.
Flags: needinfo?(dietrich)
Keywords: perf
Flags: needinfo?(mlee)
I haven't had a chance to investigate but we'll need to be very careful that the improvements are backwards-compatible with the previous version that we ship today. Otherwise we will corrupt everyone's previously-stored data...
Flags: needinfo?(mlee)
Whiteboard: [c= p= s= u=]
See Also: → 837110
I believe this is backwards compatible.  They increased the max block size from 32kb to 64kb, but this only prevents old decoders from reading from the new compressor.  We should be fine opening files compressed with the older decoder.

Since I am going to use this for the ServiceWorker Cache, I'll take on updating the snappy lib.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Not going to get to this any time soon.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Blocks: 964561
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Blocks: 1286798
Attached patch patch (obsolete) — Splinter Review
Attachment #8802694 - Flags: review?(bkelly)
Comment on attachment 8802694 [details] [diff] [review]
patch

Review of attachment 8802694 [details] [diff] [review]:
-----------------------------------------------------------------

When I spoke to bent about this way back when he suggested we should add a test that verifies IDB/Cache values stored in the old snappy can be read in the new snappy.  Do you plan to add such a test?
Attachment #8802709 - Attachment description: diff of firefox 11 snappy source and current source → diff between firefox 11 source and current m-c
So it seems there was only the initial landing (bug 703660, firefox 11) and then an update to r56 (bug 715113, firefox 12).
We have some tests in dom/indexedDB/test/unit which restore profiles w/ IDB data created in older releases of firefox, so I guess if the tests still pass with the patch in this bug, the latest version of snappy must be backwards-compatible with r56 at least.
(In reply to Ben Kelly [:bkelly] from comment #10)
> Comment on attachment 8802694 [details] [diff] [review]
> patch
> 
> Review of attachment 8802694 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When I spoke to bent about this way back when he suggested we should add a
> test that verifies IDB/Cache values stored in the old snappy can be read in
> the new snappy.  Do you plan to add such a test?

See my comment 12. For example there's now dom/indexedDB/test/unit/test_schema18upgrade.js
which restores a firefox 18 profile.
Ben, do you want me to add a test that restores a FF 11 profile ?
If that's not enough, what else is needed ?
Flags: needinfo?(bkelly)
Comment on attachment 8802694 [details] [diff] [review]
patch

Review of attachment 8802694 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, just lost track of this.  Looks good to me, but can you verify that this code is exercised in that test?  Maybe add a printf in the new snappy code and see that it runs.  I seem to recall IDB only uses snappy for certain types of values and its not clear to me if that test includes those values.
Attachment #8802694 - Flags: review?(bkelly) → review+
I'll double check, but I know for sure that IDB compresses entire structured clone data.
Thanks for the review.
Attached patch patchSplinter Review
I added a dedicated test for this.
Attachment #8802694 - Attachment is obsolete: true
Flags: needinfo?(bkelly)
Attachment #8804000 - Flags: review+
(In reply to Jan Varga [:janv] from comment #16)
> I'll double check, but I know for sure that IDB compresses entire structured
> clone data.
> Thanks for the review.

I added a dedicated test and I also added printfs to RawCompress and RawUncompress.
I saw the RawUncompress during the test, so we should be ok.
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cbca09be546
Update Snappy library to revision >= r59 to get ARM optimizations; r=bkelly
https://hg.mozilla.org/mozilla-central/rev/0cbca09be546
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(In reply to Ben Kelly [:bkelly] from comment #6)
> I believe this is backwards compatible.  They increased the max block size
> from 32kb to 64kb, but this only prevents old decoders from reading from the
> new compressor.  We should be fine opening files compressed with the older
> decoder.
> 
> Since I am going to use this for the ServiceWorker Cache, I'll take on
> updating the snappy lib.

Oh, shouldn't we increase the schema version in dom/cache/DBSchema.cpp ?
I'm increasing schema version for IDB in bug 964561 which is about to land.
Flags: needinfo?(bkelly)
Comment on attachment 8804191 [details] [diff] [review]
A follow-up fix. Increase schema version of DOM cache databases; r =bkelly

Review of attachment 8804191 [details] [diff] [review]:
-----------------------------------------------------------------

This is to prevent a Cache with the data serialized using the newer snappy from trying to be opened in a firefox that only understands the older version?  That seems reasonable.  Thanks.
Attachment #8804191 - Flags: review?(bkelly) → review+
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91152c013711
A follow-up fix. Increase schema version of DOM cache databases to prevent a Cache with the data serialized using the newer snappy from trying to be opened in a firefox that only understands the older version; r=bkelly
(In reply to Ben Kelly [:bkelly] from comment #23)
> Comment on attachment 8804191 [details] [diff] [review]
> A follow-up fix. Increase schema version of DOM cache databases; r =bkelly
> 
> Review of attachment 8804191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is to prevent a Cache with the data serialized using the newer snappy
> from trying to be opened in a firefox that only understands the older
> version?  That seems reasonable.  Thanks.

Exactly.
Flags: needinfo?(bkelly)
You need to log in before you can comment on or make changes to this bug.