The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM: IndexedDB
P3
normal
RESOLVED FIXED
5 years ago
a month ago

People

(Reporter: cpeterson, Assigned: janv)

Tracking

(Blocks: 4 bugs, {perf})

Trunk
mozilla52
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

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

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
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: ? → ---
(Reporter)

Comment 3

4 years ago
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...

Updated

4 years ago
Flags: needinfo?(mlee)
Whiteboard: [c= p= s= u=]
Blocks: 888666
(Reporter)

Updated

3 years ago
See Also: → bug 837110
Priority: -- → P3
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
Blocks: 940273
Blocks: 1110144
No longer blocks: 940273
Blocks: 1131322
No longer blocks: 1110144
Blocks: 1110136
No longer blocks: 1131322
Not going to get to this any time soon.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

5 months ago
Blocks: 964561
(Assignee)

Updated

5 months ago
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
(Assignee)

Updated

5 months ago
Blocks: 1286798
(Assignee)

Comment 8

5 months ago
Created attachment 8802694 [details] [diff] [review]
patch
Attachment #8802694 - Flags: review?(bkelly)
(Assignee)

Comment 9

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e97ea2d83ca2ea525d94d9b2fafb70fed3f6599
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?
(Assignee)

Comment 11

5 months ago
Created attachment 8802709 [details] [diff] [review]
diff between firefox 11 source and current m-c
(Assignee)

Updated

5 months ago
Attachment #8802709 - Attachment description: diff of firefox 11 snappy source and current source → diff between firefox 11 source and current m-c
(Assignee)

Comment 12

5 months ago
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.
(Assignee)

Comment 13

5 months ago
(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.
(Assignee)

Comment 14

5 months ago
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+
(Assignee)

Comment 16

5 months ago
I'll double check, but I know for sure that IDB compresses entire structured clone data.
Thanks for the review.
(Assignee)

Comment 17

5 months ago
Created attachment 8804000 [details] [diff] [review]
patch

I added a dedicated test for this.
Attachment #8802694 - Attachment is obsolete: true
Flags: needinfo?(bkelly)
Attachment #8804000 - Flags: review+
(Assignee)

Comment 18

5 months ago
(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

5 months ago
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

Comment 20

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0cbca09be546
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 21

5 months ago
(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)
(Assignee)

Comment 22

5 months ago
Created attachment 8804191 [details] [diff] [review]
A follow-up fix. Increase schema version of DOM cache databases; r =bkelly
Attachment #8804191 - Flags: review?(bkelly)
See Also: → bug 1246615
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+

Comment 24

5 months ago
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
(Assignee)

Comment 25

5 months ago
(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.
(Assignee)

Updated

5 months ago
Flags: needinfo?(bkelly)

Comment 26

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91152c013711
Depends on: 715113
Blocks: 1340901
You need to log in before you can comment on or make changes to this bug.