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

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Chris Peterson [:cpeterson]]

Fennec's and B2G's IndexedDB performance may benefit from new ARM optimizations in Snappy r59.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Chris, please measure the performance difference and renom when you have data.

Comment 3

[Chris Peterson [:cpeterson]]

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%.

Comment 4

[Dietrich Ayala (: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.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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...

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Not going to get to this any time soon.

Comment 8

[Jan Varga [:janv]]

Attachment: patch

Comment 9

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e97ea2d83ca2ea525d94d9b2fafb70fed3f6599

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 11

[Jan Varga [:janv]]

Attachment: diff between firefox 11 source and current m-c

Comment 12

[Jan Varga [:janv]]

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.

Comment 13

[Jan Varga [:janv]]

(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.

Comment 14

[Jan Varga [:janv]]

Ben, do you want me to add a test that restores a FF 11 profile ?
If that's not enough, what else is needed ?

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 16

[Jan Varga [:janv]]

I'll double check, but I know for sure that IDB compresses entire structured clone data.
Thanks for the review.

Comment 17

[Jan Varga [:janv]]

Attachment: patch

I added a dedicated test for this.

Comment 18

[Jan Varga [:janv]]

(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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cbca09be546
Update Snappy library to revision >= r59 to get ARM optimizations; r=bkelly

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cbca09be546

Comment 21

[Jan Varga [:janv]]

(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.

Comment 22

[Jan Varga [:janv]]

Attachment: A follow-up fix. Increase schema version of DOM cache databases; r =bkelly

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Jan Varga [:janv]]

(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.

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/91152c013711