Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Add explicit IDBTransaction.commit() (was: add "writeonly" mode) #234

Closed
inexorabletash opened this issue Apr 10, 2018 · 10 comments

Comments

@inexorabletash
Copy link
Member

Forked off from #34 and previous mentioned on the old wiki, but formalizing it a bit:

  • Introduce a new transaction mode, "writeonly"
  • If a transaction's mode is "writeonly":
    • any IDBObjectStore or IDBIndex operation other than put() or add() will throw NotSupportedError
    • put() and add() return null (return type changed to IDBRequest?)
    • success/error events not fired (i.e. don't bubble up to the transaction/connection)
    • a failed operation (usually, conflicting add()) still causes the transaction to abort
    • complete/abort events are still fired against the transaction

There is probably additional spec text required for clarity, but effectively since the transaction can no longer become active after the cleanup transactions steps run when the task ends, the transaction can attempt to commit immediately; it needs to wait until all of the put()s/add()s finish. The transaction lifetime steps are still a bit wishy-washy; I didn't completely revise those as part of the big 2.0 rework of the spec.

@inexorabletash
Copy link
Member Author

The big use case for this is performance:

  • general throughput - waiting for all of the success events to round-trip before committing (even if they are no-ops) can be a bottleneck
  • fire-and-forget - during shutdown/navigation/suspension it is desirable to attempt to persist data without requiring additional round-trips and stalling such a process

@inexorabletash
Copy link
Member Author

@aliams, @wanderview, @beidson - any initial feedback (not official opinions) on this idea? A non-starter, or something that would be worth the Chrome team prototyping?

@wanderview
Copy link
Member

301 @asutherland

@inexorabletash
Copy link
Member Author

FYI, I sketched out another idea a few years ago - adding an explicit commit() method to the transaction that would immediately and permanently deactivate it to prevent further requests from being filed. gist - this might be more or less appealing than a distinct mode. The down side is that success/error events are still fired against each request even if the transaction is not activated on their callbacks, and that's still unnecessary overhead in some cases.

@asutherland
Copy link
Collaborator

commit() as described at https://gist.github.com/inexorabletash/d55a6669a040e92e47c6 seems more appealing and versatile. Being unable to benefit from the optimization if you want your transaction to first assert something about the state of the database with a read limits its utility. And having a mode where events that would normally fire no longer fire and calls that would normally never throw suddenly throw doesn't help from a complexity perspective for an API many view as already overly complex.

It would of course be useful to have an idea that a commit() will be coming before the requests start getting issued. Perhaps commit() could be paired with something like beginBatch() that could hint to the implementation that it is allowed to buffer all subsequent request responses until the subsequent commit() or an also possible flush() is processed.

At least in Gecko's implementation, this would allow us to greatly optimize against janking as long as no listeners were added to the requests. Obviously, if 100 requests are dispatched and each has a listener, dispatching all 100 in a single go just before the complete/abort happens is potentially much worse than the status quo. That said, beginBatch should still allow for implementation latitude so that the 100 can be spread out over multiple logical tasks/events to avoid dominating the event loop.

@inexorabletash
Copy link
Member Author

Thanks @asutherland ! I'll pursue commit(). From our previous testing, it seems like it's a perf benefit even if the individual request events are fired, and I think it satisfies the use cases for bringing this up again (fire-and-forget at shutdown/suspend)

beginBatch() seems like it could be layered on later.

@nolanlawson
Copy link
Member

Given that the main benefit here is performance, I agree commit() may be a more direct way of tackling this problem. Adding a special mode where you can only write and can't respond to individual constraint errors may be of limited value, although it is perfect for initial database setup scenarios such as #224.

If the core performance issue is that creating too many callbacks in the form of multiple put()s causes overhead, I also think putAll() (returning some kind of array of results/errors) may also be an interesting option here.

@inexorabletash
Copy link
Member Author

Early draft PR added (#242) - any additional eyeballs on that would be welcome.

@inexorabletash inexorabletash changed the title Proposal: Add "writeonly" transaction mode Proposal: Add explicit IDBTransaction.commit() (was: add "writeonly" mode) Jun 21, 2018
@asutherland
Copy link
Collaborator

Speculative Firefox/Gecko bug tracking implementing this is https://bugzilla.mozilla.org/show_bug.cgi?id=1497007 but we're not actually ready to implement at this time.

inexorabletash added a commit that referenced this issue Jan 23, 2019
As discussed in #234, an explicit commit() 
method is added to IDBTransaction which signals that no further requests can be
made, which in turn allows implementations to initiate the commit process without
additional round trips between script's event loop and the database task queue.
@inexorabletash
Copy link
Member Author

Merged PR the spec. I'm not assuming it's perfect, but let's iterate on the spec with additional bugs, so let's close this.

Thanks to everyone for the discussion!

inexorabletash added a commit that referenced this issue Feb 1, 2021
As discussed in #234, an explicit commit() 
method is added to IDBTransaction which signals that no further requests can be
made, which in turn allows implementations to initiate the commit process without
additional round trips between script's event loop and the database task queue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants