703660 - IndexedDB: Compress structured clone data with Snappy

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

Attachment: Patch [not for checkin]

I've been thinking that we should try to use Snappy (https://code.google.com/p/snappy) for a while and so I finally ran some numbers. Our test case was basically adding 5000 entries into an object store. Each of the entries has a data member with either 32k or 1k worth of string data. Times for adding the data and the final file sizes are:

==========
No Snappy:
==========

Storing 32k fragments:

Put 5001 ids in 3617 ms
Put 5001 ids in 3728 ms
Put 5001 ids in 3724 ms
Put 5001 ids in 3662 ms
Final size: 481728k

Storing 1k fragments:

Put 5001 ids in 631 ms
Put 5001 ids in 615 ms
Put 5001 ids in 668 ms
Put 5001 ids in 666 ms
Final size: 17600k

=============
Using Snappy:
=============

Storing 32k fragments:

Put 5001 ids in 4416 ms
Put 5001 ids in 4538 ms
Put 5001 ids in 4437 ms
Put 5001 ids in 4464 ms
Final size: 54912k

Storing 1k fragments:

Put 5001 ids in 776 ms
Put 5001 ids in 804 ms
Put 5001 ids in 795 ms
Put 5001 ids in 796 ms
Put 5001 ids in 772 ms
Final size: 4192k

That's an order of magnitude difference in file size for the large data case! The slowdown is unfortunate, but I think it's probably worth it. We'll need to think through data migration from old databases a little, but that shouldn't be too hard.

Posting the patch here now so that I don't lose it. The patch also doesn't include the snappy source files just to keep the size down.

Comment 2

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

Attachment: Patch, v1 [not for checkin, missing snappy source]

This patch is the whole thing (minus the snappy source). Everything seems to work well! Migrates old databases to the new compression format, changes how we do data versioning too.

Comment 3

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

Matt, heads up, I've gone ahead and integrated Snappy here.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 575803 [details] [diff] [review]
Patch, v1 [not for checkin, missing snappy source]

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

r=me on the non-build-goop stuff.

Though this version stuff is way too complex :(

::: dom/indexedDB/IDBObjectStore.cpp
@@ +755,5 @@
> +    NS_WARNING("Snappy can't determine uncompressed length!");
> +    return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +  }
> +
> +  return aBuffer.copy(reinterpret_cast<const uint64_t *>(uncompressed.get()),

Please file a bug on avoiding the copy here.

Comment 5

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

Comment on attachment 575803 [details] [diff] [review]
Patch, v1 [not for checkin, missing snappy source]

khuey, can you review the build goop here?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 575803 [details] [diff] [review]
Patch, v1 [not for checkin, missing snappy source]

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

Do we need to add something to toolkit/content/license.html here?

r=me on the "build goop stuff"

::: other-licenses/snappy/Makefile.in
@@ +41,5 @@
> +
> +VPATH = \
> +  @srcdir@ \
> +  $(topsrcdir)/other-licenses/snappy/src
> +  $(NULL)

VPATH += $(topsrcdir)/other-licenses/snappy/src should be enough

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> ::: other-licenses/snappy/Makefile.in
> @@ +41,5 @@
> > +
> > +VPATH = \
> > +  @srcdir@ \
> > +  $(topsrcdir)/other-licenses/snappy/src
> > +  $(NULL)
> 
> VPATH += $(topsrcdir)/other-licenses/snappy/src should be enough

Actually, ignore me, I'm being silly.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7634808d94af

Comment 9

[Ed Morley [:emorley]]

Backed out of inbound for PGO Linux64 build failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7634808d94af
https://tbpl.mozilla.org/php/getParsedLog.php?id=7527408&tree=Mozilla-Inbound
{
/tools/python/bin/python2.5 /builds/slave/m-in-lnx64-pgo/build/config/pythonpath.py -I../../config /builds/slave/m-in-lnx64-pgo/build/config/expandlibs_exec.py --uselist --  /usr/bin/ccache /tools/gcc-4.5/bin/g++  -fno-rtti -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -Wno-long-long -fno-exceptions -fno-strict-aliasing -std=gnu++0x -pthread -pipe  -DNDEBUG -DTRIMMED -g -fprofile-generate -O3 -fomit-frame-pointer -fPIC -shared -Wl,-z,defs -Wl,-h,libxul.so -o libxul.so  stdc++compat.i_o nsStaticXULComponents.i_o nsUnicharUtils.i_o nsBidiUtils.i_o nsRDFResource.i_o    -lpthread    -fprofile-generate -Wl,-rpath-link,/builds/slave/m-in-lnx64-pgo/build/obj-firefox/dist/bin -Wl,-rpath-link,/usr/local/lib    ../../toolkit/xre/libxulapp_s.a  ../../staticlib/components/libnecko.a ../../staticlib/components/libuconv.a ../../staticlib/components/libi18n.a ../../staticlib/components/libchardet.a ../../staticlib/components/libjar50.a ../../staticlib/components/libstartupcache.a ../../staticlib/components/libpref.a ../../staticlib/components/libhtmlpars.a ../../staticlib/components/libimglib2.a ../../staticlib/components/libgkgfx.a ../../staticlib/components/libgklayout.a ../../staticlib/components/libdocshell.a ../../staticlib/components/libembedcomponents.a ../../staticlib/components/libwebbrwsr.a ../../staticlib/components/libnsappshell.a ../../staticlib/components/libtxmgr.a ../../staticlib/components/libcommandlines.a ../../staticlib/components/libtoolkitcomps.a ../../staticlib/components/libpipboot.a ../../staticlib/components/libpipnss.a ../../staticlib/components/libappcomps.a ../../staticlib/components/libjsreflect.a ../../staticlib/components/libcomposer.a ../../staticlib/components/libjetpack_s.a ../../staticlib/components/libtelemetry.a ../../staticlib/components/libjsdebugger.a ../../staticlib/components/libstoragecomps.a ../../staticlib/components/librdf.a ../../staticlib/components/libwindowds.a ../../staticlib/components/libjsctypes.a ../../staticlib/components/libjsperf.a ../../staticlib/components/libgkplugin.a ../../staticlib/components/libunixproxy.a ../../staticlib/components/libjsd.a ../../staticlib/components/libautoconfig.a ../../staticlib/components/libauth.a ../../staticlib/components/libcookie.a ../../staticlib/components/libpermissions.a ../../staticlib/components/libuniversalchardet.a ../../staticlib/components/libfileview.a ../../staticlib/components/libplaces.a ../../staticlib/components/libtkautocomplete.a ../../staticlib/components/libsatchel.a ../../staticlib/components/libpippki.a ../../staticlib/components/libwidget_gtk2.a ../../staticlib/components/libsystem-pref.a ../../staticlib/components/libimgicon.a ../../staticlib/components/libaccessibility.a ../../staticlib/components/libremoteservice.a ../../staticlib/components/libspellchecker.a ../../staticlib/components/libzipwriter.a ../../staticlib/components/libservices-crypto.a ../../staticlib/libjsipc_s.a ../../staticlib/libdomipc_s.a ../../staticlib/libdomplugins_s.a ../../staticlib/libmozipc_s.a ../../staticlib/libmozipdlgen_s.a ../../staticlib/libipcshell_s.a ../../staticlib/libgfx2d.a ../../staticlib/libgfxipc_s.a ../../staticlib/libhal_s.a ../../staticlib/libxpcom_core.a ../../staticlib/libucvutil_s.a ../../staticlib/libchromium_s.a ../../staticlib/libmozreg_s.a ../../staticlib/libsnappy_s.a ../../staticlib/libgtkxtbin.a ../../staticlib/libthebes.a ../../staticlib/libgl.a ../../staticlib/libycbcr.a ../../staticlib/libangle.a  -L../../dist/bin -L../../dist/lib ../../media/libjpeg/libmozjpeg.a ../../media/libpng/libmozpng.a ../../gfx/qcms/libmozqcms.a /builds/slave/m-in-lnx64-pgo/build/obj-firefox/dist/lib/libjs_static.a -L../../dist/bin -L../../dist/lib -lcrmf -lsmime3 -lssl3 -lnss3 -lnssutil3 ../../gfx/cairo/cairo/src/libmozcairo.a  ../../gfx/cairo/libpixman/src/libmozlibpixman.a  -L/usr/lib64 -lXrender -lfreetype -lfontconfig ../../gfx/harfbuzz/src/libmozharfbuzz.a ../../gfx/ots/src/libmozots.a  ../../dist/lib/libmozsqlite3.a  ../../modules/zlib/src/libmozz.a -lasound -lm -ldl -lpthread   -lrt -L../../dist/bin -L../../dist/lib  -L/builds/slave/m-in-lnx64-pgo/build/obj-firefox/dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl ../../dist/lib/libmozalloc.a -L/lib64 -ldbus-glib-1 -ldbus-1 -lglib-2.0   -L/usr/lib64 -lX11  -lXext  -L/lib64 -lpangoft2-1.0 -lpangocairo-1.0 -lpango-1.0 -lcairo -lgobject-2.0 -lgmodule-2.0 -ldl -lglib-2.0   -L/lib64 -lgtk-x11-2.0 -latk-1.0 -lgdk-x11-2.0 -lgdk_pixbuf-2.0 -lm -lpangocairo-1.0 -lpango-1.0 -lcairo -lgobject-2.0 -lgmodule-2.0 -ldl -lglib-2.0   -lXt -lgthread-2.0 -lfreetype -ldl  -lrt    
/usr/bin/ld: ../../other-licenses/snappy/snappy.i_o: relocation R_X86_64_PC32 against `std::__throw_bad_cast()' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: Bad value
collect2: ld returned 1 exit status
make[6]: *** [libxul.so] Error 1
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/30d495095e2b

Comment 10

[Jan Varga [:janv]]

Comment on attachment 575803 [details] [diff] [review]
Patch, v1 [not for checkin, missing snappy source]

>-  // Get the data version.
>-  nsCOMPtr<mozIStorageStatement> stmt;
>-  rv = connection->CreateStatement(NS_LITERAL_CSTRING(
>-    "SELECT dataVersion "
>-    "FROM database"
>-  ), getter_AddRefs(stmt));
>+  if (!mForDeletion) {
>+    // Get the data version, and maybe vacuum if we upgrade (hopefully because
>+    // we figured out a way to save some disk space).
>+    PRInt64 dataVersion;
>+    rv = GetDataVersion(connection, &dataVersion);
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
>+
>+    // This logic needs to change next time we change the data version!
>+    PR_STATIC_ASSERT(kJSStructuredCloneVersion == 1);
>+    PR_STATIC_ASSERT(kInternalDataVersion == 1);
>+
>+    bool vacuumNeeded = false;
>+
>+    if (dataVersion != kDataVersion && !mForDeletion) {

redundant !mForDeletion check ?

I'm not checking all your patches :)
I'm just updating files in idb patch and found it by chance.

Comment 11

[Jan Varga [:janv]]

Attachment: patch updated to latest trunk

Comment 12

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

Attachment: Patch, final

Final patch.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 579252 [details] [diff] [review]
Patch, final

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

r=me with that fixed

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +75,5 @@
> +PRInt32
> +MakeSchemaVersion(PRUint32 aMajorSchemaVersion,
> +                  PRUint32 aMinorSchemaVersion)
> +{
> +  return PRInt32((aMajorSchemaVersion << 0x8) + aMinorSchemaVersion);

should be 4, no 0x8

@@ +78,5 @@
> +{
> +  return PRInt32((aMajorSchemaVersion << 0x8) + aMinorSchemaVersion);
> +}
> +
> +const PRInt32 kSQLiteSchemaVersion = PRInt32((kMajorSchemaVersion << 0x8) +

Same here

@@ +84,5 @@
> +
> +inline
> +PRUint32 GetMajorSchemaVersion(PRInt32 aSchemaVersion)
> +{
> +  return PRUint32(aSchemaVersion) >> 0x8;

And here

@@ +1010,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  if (schemaVersion > kSQLiteSchemaVersion) {
> +    NS_WARNING("Unable to open IndexedDB database, schema is too high!");
> +    return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;

Can you file a bug on firing a more specific error here

@@ +1043,5 @@
> +      NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +    }
> +    else  {
> +      // This logic needs to change next time we change the schema!
> +      PR_STATIC_ASSERT(kSQLiteSchemaVersion == PRInt32((9 << 0x8) + 0));

4 again

@@ +1050,5 @@
> +        if (schemaVersion == 4) {
> +          rv = UpgradeSchemaFrom4To5(connection);
> +        }
> +        else if (schemaVersion == 5) {
> +          rv = UpgradeSchemaFrom5To6(connection);

This won't work. Once we've upgraded to 4->5 we should upgrade to 5->6 no?

I think you need to just remove the |else|

@@ +1094,5 @@
>    }
>  
>    // Turn on foreign key constraints.
>    rv = connection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>      "PRAGMA foreign_keys = ON;"

Might be worth putting the foreign_keys=OFF at the top of the upgrade code so that we don't have to remember to do that everywhere. Otherwise some upgrade paths which set it would work, while upgrading from newer version might wipe all data

Comment 14

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

(In reply to Jonas Sicking (:sicking) from comment #13)
> should be 4, no 0x8

Blegh. Fixed.

> Can you file a bug on firing a more specific error here

Do we really care? This isn't likely to be a problem for anyone other than nightly testers...

> This won't work. Once we've upgraded to 4->5 we should upgrade to 5->6 no?

It's in a loop so this will work fine.

> Might be worth putting the foreign_keys=OFF at the top of the upgrade code

It's off by default, I'll just remove these.

Comment 15

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

Filed bug 708158 about the crazy linux pgo failure.

Comment 16

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

Attachment: Fix linux PGO

This is needed to fix linux PGO. Apparently it was un-inlining these functions. Thanks to cjones and jlebar for helping me debug.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 579603 [details] [diff] [review]
Fix linux PGO

r=me with s/inline/NS_ALWAYS_INLINE/ per discussion.

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8941e2b7a0bf

Comment 19

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f0ec491b9e for Windows PGO build failures. Suspicious as hell that we spent all day yesterday with a "nswindowmediator.cpp(821) : fatal error C1001: An internal error has occurred in the compiler. LINK : fatal error LNK1000: Internal error during IMAGE::BuildImage" failure too, but once we backed out bug 676349 we did go back to being able to build PGO. Twice. Then this landed. We'll see whether this backout also manages to cure it.

Comment 20

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

(In reply to Phil Ringnalda (:philor) from comment #19)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f0ec491b9e for
> Windows PGO build failures.

Wait, the very next build succeeded, didn't it?

https://hg.mozilla.org/integration/mozilla-inbound/rev/76a185935937

Comment 21

[Phil Ringnalda (:philor)]

Odd. Strictly speaking, no, because those "PGO" tests are from the nightly, rather than a periodic PGO build, but nightlies are PGO'ed, and as far as I know, the only difference between them and periodic PGO is that they always clobber. Possible that I need to clobber the piss out of things, and retrigger dozens of PGO builds.

Comment 22

[Phil Ringnalda (:philor)]

Clobber set, PGO retriggered on your push, should know in 4-6 hours.

Comment 23

[Phil Ringnalda (:philor)]

Sadly, clobbering doesn't seem to have been the answer, the second one in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=8941e2b7a0bf says it was a clobber, and still had the same compiler crash.

Comment 24

[Phil Ringnalda (:philor)]

While the clobber of the bug 676349 push was green, so I don't have the slightest hint of a clue.

Comment 25

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

Since it doesn't really seem like this patch is the culprit I went ahead and relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd4e34b2f6f.

Comment 26

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

And subsequent builds on the original push look fine. Solar wind, perhaps?

https://tbpl.mozilla.org/php/getParsedLog.php?id=7811571&tree=Mozilla-Inbound&full=1

Comment 27

[Ed Morley [:emorley]]

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

Comment 28

[[e-mail]]

snappy.cc doesn't compile under gcc4.3.2 because of extraneous semicolons :-(