Closed Bug 984131 Opened 7 years ago Closed 7 years ago

Async transactions: protect transactions objects so we can optimize them


(Toolkit :: Places, defect)

Not set





(Reporter: mano, Assigned: mano)



(Whiteboard: p=2 s=it-31c-30a-29b.2 [qa-])


(1 file, 1 obsolete file)

Attached patch protectTransactions.diff (obsolete) — Splinter Review
Learning from our mistakes, I want to make the new PlacesTransactions implementation smarter about undoing batched transactions. For instance, consider a copy-paste-ui-action of a folder with few levels of nesting. Undoing that batch of transaction should ideally involve a single call to removeItem. However, both the old and the new API would invoke removeItem for each nestedly-created item. 

I'm not yet sure how I want to implement such an optimization, but one thing is certain: transaction would have to access some internal properties of each other. That doesn't play nicely with the way I'm hiding everything inside the |execute| method of each transaction. I did that for a reason though: consumers shouldn't be able to mess with transaction internals. Ideally, there should be no way for consumers to break the transaction history apart using raw places APIs.

So, I think the best way to approach this is to "proxify" transactions. Consumers won't have any ability to access transactions' internals.  While doing that, I'm also going to completely remove the ability to implement custom transactions (instead of just warning consumers not to do so) and make it so a transactions cannot be "recycled". All of this changes together should allow us to safely optimize batched changes.

The attached patch is somewhat tentative. I don't like the WeakMap usage stuff and I'm open to suggestions for better tricks.
Attachment #8391900 - Flags: review?(mak77)
Oh, and I also found and fixed a bug wrt. to "nested" generators.
Attachment #8391900 - Attachment is obsolete: true
Attachment #8391900 - Flags: review?(mak77)
Comment on attachment 8392264 [details] [diff] [review]

Review of attachment 8392264 [details] [diff] [review]:

not extremely happy there's this additional hop in the internal code, but can't think of a better solution atm

::: toolkit/components/places/PlacesTransactions.jsm
@@ +153,5 @@
>                       this[this.undoPosition - 1] : null,
> +  // Outside of this module, the API of transactions is inaccessible, and so
> +  // are any internal properties.  To achieve that, transactions are proxified
> +  // in their constructors.  This map maps the proxies to the raw objects.

nit: s/This map maps/This maps/
s/the raw objects/their respective raw objects/

@@ +161,5 @@
> +   * Proxify a transaction object for consumers.
> +   * @param aRawTransaction
> +   *        the raw transaction object.
> +   * @return an object that can be passed to getRawTransaction for retrieving
> +   * aRawTransaction.

nit: indent "aRawTransaction" as much as "an object"

I think the fact one may use getRawTransaction to get the raw transaction should go into a @note, while here should just be @return proxified transaction object

@@ +388,2 @@
>            sendValue = next.value;
> +          if ( == "[object Generator]") {

can we use isGenerator here or do we need this stricter check?
Attachment #8392264 - Flags: review?(mak77) → review+
isGenerator is confusing. It checks if a function is a generator function. The check you commented about checks if the object in question is a generator object (the "return value" of a generator function).
This is in parallel with Task.jsm. Task.spawn takes a generator function, but for "sub-spawning" you yield generators, not generator-functions.
I see
Whiteboard: p=0
Whiteboard: p=0 → p=2 s=it-31c-30a-29b.2
Whiteboard: p=2 s=it-31c-30a-29b.2 → p=2 s=it-31c-30a-29b.2 [qa-]
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.