Jan Varga [:janv] Reporter
	Description • 12 years ago

      No description provided.

	Jan Varga [:janv] Reporter
	Comment 1 • 12 years ago

The implementation will be done as a diploma thesis, here's the goal of the thesis:

The goal of this work is to implement the IndexedDB synchronous API in Firefox browser and in the new mobile platform Firefox OS. These platforms already support the asynchronous API of the database, but the missing implementation of the synchronous API, which is currently unsupported in the other known web browsers too, partially prevents broader deployment and increase of popularity of IndexedDB among developers of web applications.

1. Describe HTML5 applications in general and local storage of data (especially IndexedDB database)

2. Implementation of the synchronous IndexedDB API in Firefox / Firefox OS

3. Describe the implementation in terms of architecture, description of objects, interactions between them as well as description of communication between threads.

4. Evaluation of results

	Vendelin Ruzicka [:vendo]
	Comment 3 • 12 years ago

New attachment adds WebIDL files to dom/webidl and *.h *.cpp files to dom/workers with stub methods for sync API of IDB.

DOMStringList attributes and parameters are commented out because they are not yet available (see bug 803106 ) and the same with callbacks (see bug 812783 ).

	Vendelin Ruzicka [:vendo]
	Comment 5 • 11 years ago

The actual state of work.
In the case of testing this patch need to be applied on top of the ipcthreadbase patch from bug 809888

Methods(m)/properties(p) which mostly work:

IDBDatabaseSync
 p: name,version,objectStoreNames
 m: transaction

IDBFactorySync
 m: open, cmp
 
IDBTransactionSync
 p: mode, db,
 m: objectStore

IDBObjectStoreSync
 p: transaction,
 m: get, delete, clear, index, count

IDBIndexSync
 p: objectStore
 m: openKeyCursor, get, getKey, count
 
IDBCursorSync
 p: source, direction, key, primaryKey
 m: continue

	Jan Varga [:janv] Reporter
	Comment 6 • 11 years ago

Vendo is going to submit a new patch soon. The implementation is almost feature complete.

Stuff that needs to be implemented:
IDBFactory::DeleteDatabase()
IDBTransactionSync::Error()
IDBTransactionSync::Abort()
onversionchange handling

	Vendelin Ruzicka [:vendo]
	Comment 7 • 11 years ago

For more information about current state of implementation visit:
https://etherpad.mozilla.org/sync-idb

	Vendelin Ruzicka [:vendo]
	Comment 8 • 11 years ago

This version of the synchronous IDB API patch was officially attached to the diploma thesis (final version for university).

	David Teller [:Yoric] - still alive but not very active
	Comment 9 • 11 years ago

Hi, vendo.
It would be great to have this feature. Could you please divide the patch into smaller chunks, so as to simplify the work of reviewers?

	Jan Varga [:janv] Reporter
	Comment 10 • 11 years ago

I just finished work on bringing the sync IDB patch up to date and I also rebased it on top of the temp storage, bug 785884.

Sync IDB is a Q3 goal and we will be talking about it during the work week in Toronto.
I've started working on an initial review and Ben Turner will do a super review after the first one is addressed.

We will split the patch into two parts at least, the implementation and the tests. There's actually a standalone patch already, bug 809888

	Jan Varga [:janv] Reporter
	Comment 11 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] <on workweek, will not follow bugzilla until July 1st> from comment #9)
> Hi, vendo.
> It would be great to have this feature. Could you please divide the patch
> into smaller chunks, so as to simplify the work of reviewers?

I forgot to ask, what is your use case of sync IDB ?

	David Teller [:Yoric] - still alive but not very active
	Comment 12 • 11 years ago

> I just finished work on bringing the sync IDB patch up to date and I also rebased it on top of the temp storage, bug 785884.

Ah, great. I was a bit worried because I saw a huge patch without a review flag, so I was afraid it would stay here and bitrot.

> I forgot to ask, what is your use case of sync IDB ?

I have no immediate use case, but I'm tech leading the Async project [1] and I'd like to evaluate sync IDB to determine whether I should suggest this as a storage solution for add-on authors.

[1] https://bugzilla.mozilla.org/buglist.cgi?cmdtype=runnamed&namedcmd=Async

	Jan Varga [:janv] Reporter
	Comment 13 • 11 years ago

I just found this:
"Rationale:  Workers are coming along great, but of course it would be great to have more: IndexedDB access, mozTCPSocket access, better debug and profile support for them.
Rationale for Gaia:  Email app uses a worker with moxTCPSocket and IndexedDB. Right now it proxies back to the main thread to do IO with those capabilities. It complicates the code. It is also difficult to debug issues in the workers as well as get accurate info on profiling what happens in them."

The current implementation of sync IDB API is single process only, we will need to add support for child processes too.

	Andrew Sutherland [:asuth] (he/him)
	Comment 14 • 11 years ago

(In reply to Jan Varga [:janv] from comment #13)
> Rationale for Gaia:  Email app uses a worker with moxTCPSocket and
...
> The current implementation of sync IDB API is single process only, we will
> need to add support for child processes too.

For Gaia, I think it's always expected that each app (with its own origin) will only ever operate within a single process, so that might eliminate that concern.

For the Gaia e-mail app (in regards to that blurb), we'd really prefer the async API because of our architecture.  But we can use the sync API in a sub-worker so that we're remoting to another background thread rather than the main thread.

	Jan Varga [:janv] Reporter
	Comment 15 • 11 years ago

(In reply to Andrew Sutherland (:asuth) from comment #14)
> (In reply to Jan Varga [:janv] from comment #13)
> > Rationale for Gaia:  Email app uses a worker with moxTCPSocket and
> ...
> > The current implementation of sync IDB API is single process only, we will
> > need to add support for child processes too.
> 
> For Gaia, I think it's always expected that each app (with its own origin)
> will only ever operate within a single process, so that might eliminate that
> concern.

I actually meant that the current implementation works in Firefox Desktop only (one single process) and that will need to add support for workers created from child processes.

> 
> For the Gaia e-mail app (in regards to that blurb), we'd really prefer the
> async API because of our architecture.  But we can use the sync API in a
> sub-worker so that we're remoting to another background thread rather than
> the main thread.

ok, thanks for this info

	David Bruant
	Comment 16 • 11 years ago

(In reply to Andrew Sutherland (:asuth) from comment #14)
> For the Gaia e-mail app (in regards to that blurb), we'd really prefer the
> async API because of our architecture.  But we can use the sync API in a
> sub-worker so that we're remoting to another background thread rather than
> the main thread.
This wording worries me a bit. "But we can use the sync API in a sub-worker" suggests that you could also decide otherwise (that is using the sync API in the main thread). Last I discussed this with Jonas Sicking [1] on this topic, it was clear that this cannot be an option.

With the current implementation (or at least intent on this bug), will it be possible to use the sync API on the main thread?

[1] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0648.html and following messages

	Andrew Sutherland [:asuth] (he/him)
	Comment 17 • 11 years ago

(In reply to David Bruant from comment #16)
> This wording worries me a bit. "But we can use the sync API in a sub-worker"
> suggests that you could also decide otherwise (that is using the sync API in
> the main thread). Last I discussed this with Jonas Sicking [1] on this
> topic, it was clear that this cannot be an option.

It is indeed my understanding that the sync API would never be exposed to the main thread under any circumstances.

What I was trying to express is that for the e-mail app, what we really want is async access to IndexedDB on a worker thread.  We currently use only one worker and having it block on I/O would be very bad.  But it seems like we could cobble together an async IndexedDB impementation out of a sub-worker and a synchronous-on-workers-only IndexedDB implementation if that is all we have.  Currently, we see a non-trivial amount of main-thread jank (on a single-core non-beefy Firefox OS device) just from the structured clone operations bouncing our calls to the async IndexedDB off the main thread.

	Jan Varga [:janv] Reporter
	Comment 18 • 11 years ago

(In reply to David Bruant from comment #16)
> With the current implementation (or at least intent on this bug), will it be
> possible to use the sync API on the main thread?

of course not :)

using the sync API in a subworker should be ok

	Jan Varga [:janv] Reporter
	Comment 19 • 11 years ago

A rollup patch for those who want to try it with the current m-c.

https://tbpl.mozilla.org/?tree=Try&rev=b0246a1406d2

	Jan Varga [:janv] Reporter
	Comment 20 • 11 years ago

This one actually builds on windows:
https://tbpl.mozilla.org/?tree=Try&rev=a932c801c1d8

	Jan Varga [:janv] Reporter
	Comment 21 • 11 years ago

Additional development like review fixes, support for stored files, access from workers in child processes, etc. will be done on a disposable project branch.
https://tbpl.mozilla.org/?tree=Jamun

	Jan Varga [:janv] Reporter
	Comment 22 • 11 years ago

Comment on attachment 746580 [details] [diff] [review]
Sync IDB API (diploma thesis version)

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

::: dom/workers/DatabaseInfoSync.h
@@ +12,5 @@
> +#include "mozilla/dom/indexedDB/DatabaseInfo.h"
> +
> +BEGIN_WORKERS_NAMESPACE
> +
> +struct DatabaseInfoSync : public indexedDB::DatabaseInfoBase

This should be called DatabaseInfoMT and the hashtable should be protected by a mutex.
It would be also nice to have a test that creates multiple workers which access the same database.

	Jan Varga [:janv] Reporter
	Comment 23 • 11 years ago

Comment on attachment 746580 [details] [diff] [review]
Sync IDB API (diploma thesis version)

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

::: dom/webidl/IDBCursorSync.webidl
@@ +6,5 @@
> + * The origin of this IDL file is http://www.w3.org/TR/IndexedDB/
> + */
> +
> +interface IDBCursorSync {
> +  readonly attribute object source;

We decided to follow the style used in the specification [1], bug 888598.
Please adjust all the webidl files to match the spec.

[1] http://www.w3.org/TR/2012/WD-IndexedDB-20120524/

::: dom/webidl/IDBDatabaseSync.webidl
@@ +36,5 @@
> +              optional unsigned long timeout);
> +
> +  [Throws]
> +  /* FileHandleSync */ any
> +  mozCreateFileHandle(DOMString name,

This is a mozilla specific method, thus it should be move to "partial interface IDBDatabaseSync" (see how it is done elsewhere).

Also, you can remove the "moz" prefix in the implementation, take a look at 'binaryNames' for IDBIndex in Bindings.conf
I think you can do that for IDBDatabase interface too.

.mozStorage in IDBDatabaseSync should be moved to a partial interface too and make it pref controlled please

	Jan Varga [:janv] Reporter
	Comment 24 • 11 years ago

Comment on attachment 792717 [details] [diff] [review]
A rollup patch

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

::: dom/bindings/Codegen.py
@@ +3397,5 @@
> +                # We have a specialization of Optional that will use a
> +                # Rooted for the storage here.
> +                declType = CGGeneric("JS::Handle<JSObject*>")
> +            else:
> +                declType = CGGeneric("JS::Rooted<JSObject*>")

Vendo, please file a new bug for this and cc bz. This is actually a followup for bug 868715 part 10, code for callbacks in workers hasn't been updated properly I think.

	Jan Varga [:janv] Reporter
	Comment 26 • 11 years ago

Comment on attachment 808274 [details] [diff] [review]
diff between m-c and jamun

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

::: dom/workers/DatabaseInfoMT.h
@@ +29,5 @@
> +  already_AddRefed<DatabaseInfoMT> Clone();
> +
> +  nsCString id;
> +
> +  static StaticMutex sDatabaseInfoMutex;

Vendo, the mutex only protects Get/Put/Remove at the moment, we have to protect other methods too.
For example GetObjectStore(), take a look at this crash:
https://tbpl.mozilla.org/php/getParsedLog.php?id=28204915&tree=Jamun&full=1#error1

	Jan Varga [:janv] Reporter
	Comment 27 • 11 years ago

Comment on attachment 808274 [details] [diff] [review]
diff between m-c and jamun

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

::: dom/workers/IDBDatabaseSync.cpp
@@ +524,5 @@
> +
> +  if (mUpgradeNeeded) {
> +    if (aUpgradeCallback) {
> +      ErrorResult rv;
> +      aUpgradeCallback->Call(mFactory, *mTransaction, 99, rv);

Vendo, we need to pass a real number here and it seems we don't have a test for it.
Async test suite has it in test_setVersion_events.js

	Jan Varga [:janv] Reporter
	Comment 28 • 11 years ago

Vendo, another thing ... please convert all NS_ASSERTION that we added to MOZ_ASSERT

	Jan Varga [:janv] Reporter
	Comment 29 • 11 years ago

It would be also nice to have getAllKeys and openKeyCursor in IDBObjectStoreSync. These were added to async API in bug 920633 and bug 920800.

	Jan Varga [:janv] Reporter
	Comment 30 • 11 years ago

IDBKeyRange is now shared by the main thread and workers

	Jan Varga [:janv] Reporter
	Comment 31 • 11 years ago

Comment on attachment 808274 [details] [diff] [review]
diff between m-c and jamun

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

::: dom/workers/WorkerScope.cpp
@@ +1180,3 @@
>        !EventBinding::GetConstructorObject(aCx, global) ||
>        !FileReaderSyncBinding_workers::GetConstructorObject(aCx, global) ||
> +      !IDBCursorSyncBinding_workers::GetProtoObject(aCx, global) ||

Vendo, all these GetProtoObject() should be changed to GetConstructorObject()

::: dom/workers/test/indexedDB/objectCursors_worker.js
@@ +57,5 @@
> +
> +    }
> +  }
> +
> +  ok(true, "Test successfully completed");

This can be: info("Test successfully completed"), you will need to add info() the helpers.

	Vendelin Ruzicka [:vendo]
	Comment 32 • 11 years ago

Most of the reviewers comments has been already addressed on jamun.
I had a wonderful time at the Brussels summit and fixed comment#31 only today.
OpenKeyCursor function will be added soon to IDBObjectStoreSync.

	Vendelin Ruzicka [:vendo]
	Comment 33 • 11 years ago

OpenKeyCursor function has been added.

	Jan Varga [:janv] Reporter
	Comment 35 • 11 years ago

Just to bring some context, we're currently pondering the possibility to quickly rewrite the sync IDB impl using Ben's new event loop and direct IPC in workers once it's ready or just let Ben review the current patch.

Vendo said he's available to help with the rewrite.

	Jan Varga [:janv] Reporter
	Comment 37 • 10 years ago

Update:
The current plan is to land existing patch, pref it off, get feedback, enable it once some other vendor is willing to implement it

	Andrew Sutherland [:asuth] (he/him)
	Comment 38 • 6 years ago

The synchronous API didn't happen and is definitely no longer going to happen.