Last Comment Bug 703660 - IndexedDB: Compress structured clone data with Snappy
: IndexedDB: Compress structured clone data with Snappy
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
https://code.google.com/p/snappy
: 686314 715019 (view as bug list)
Depends on: 708158 709057
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-18 10:54 PST by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2012-03-22 11:54 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch [not for checkin] (11.09 KB, patch)
2011-11-18 10:54 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Splinter Review
Patch, v1 [not for checkin, missing snappy source] (35.82 KB, patch)
2011-11-20 21:42 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
jonas: review+
khuey: review+
Details | Diff | Splinter Review
patch updated to latest trunk (249.48 KB, patch)
2011-12-05 21:55 PST, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
Patch, final (249.14 KB, patch)
2011-12-06 00:24 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
jonas: review+
Details | Diff | Splinter Review
Fix linux PGO (2.66 KB, patch)
2011-12-06 23:22 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-18 10:54:58 PST
Created attachment 575508 [details] [diff] [review]
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 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-18 13:00:10 PST
*** Bug 686314 has been marked as a duplicate of this bug. ***
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-20 21:42:02 PST
Created attachment 575803 [details] [diff] [review]
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!) 2011-11-21 14:59:53 PST
Matt, heads up, I've gone ahead and integrated Snappy here.
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-21 17:25:34 PST
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!) 2011-11-21 18:34:14 PST
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-21 18:56:01 PST
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-21 18:59:21 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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!) 2011-11-21 20:22:23 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7634808d94af
Comment 9 Ed Morley [:emorley] 2011-11-22 03:57:57 PST
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] 2011-11-22 08:08:25 PST
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] 2011-12-05 21:55:01 PST
Created attachment 579229 [details] [diff] [review]
patch updated to latest trunk
Comment 12 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-06 00:24:53 PST
Created attachment 579252 [details] [diff] [review]
Patch, final

Final patch.
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-06 00:44:01 PST
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!) 2011-12-06 19:12:19 PST
(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!) 2011-12-06 19:12:55 PST
Filed bug 708158 about the crazy linux pgo failure.
Comment 16 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-06 23:22:11 PST
Created attachment 579603 [details] [diff] [review]
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 2011-12-07 00:07:22 PST
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!) 2011-12-07 00:22:51 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/8941e2b7a0bf
Comment 19 Phil Ringnalda (:philor) 2011-12-07 08:44:42 PST
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!) 2011-12-07 09:34:10 PST
(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) 2011-12-07 09:42:10 PST
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) 2011-12-07 10:08:24 PST
Clobber set, PGO retriggered on your push, should know in 4-6 hours.
Comment 23 Phil Ringnalda (:philor) 2011-12-07 13:16:21 PST
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) 2011-12-07 14:21:17 PST
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!) 2011-12-07 15:27:03 PST
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!) 2011-12-07 19:08:06 PST
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] 2011-12-08 08:44:03 PST
https://hg.mozilla.org/mozilla-central/rev/0dd4e34b2f6f
Comment 28 neil@parkwaycc.co.uk 2011-12-11 05:54:51 PST
snappy.cc doesn't compile under gcc4.3.2 because of extraneous semicolons :-(
Comment 29 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-04 06:03:34 PST
*** Bug 715019 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.