Bug 73389 – move data in IDBPendingTransactionMonitor from static to ThreadLocal

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

move data in IDBPendingTransactionMonitor from static to ThreadLocal

[David Grogan]

Created attachment 117107 [details]
Patch

[David Grogan]

Dave Levin, could you review this?

[David Levin]

Comment on attachment 117107 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117107&action=review

Feel free to address the items I mentioned and land it.

> Source/WebCore/storage/IDBPendingTransactionMonitor.cpp:38
> +    AtomicallyInitializedStatic(ThreadSpecific<Vector<IDBTransactionBackendInterface*> >*, transactions = new ThreadSpecific<Vector<IDBTransactionBackendInterface*> >);

Note AtomicallyInitializedStatic takes a lock every time it is hit (every time you all transactions()).

> Source/WebCore/storage/IDBPendingTransactionMonitor.cpp:49
> +    size_t pos = transactions()->find(transaction);

You should consider storing transactions() in a local variable to avoid hit the mutex so often (especially in the loop below, etc).

> Source/WebCore/storage/IDBPendingTransactionMonitor.cpp:59
> +        transactions()->at(i)->abort();

Shouldn't this also remove all of the items in addition to aborting them (transactions().clear())?

[David Grogan]

Comment on attachment 117107 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117107&action=review

>> Source/WebCore/storage/IDBPendingTransactionMonitor.cpp:49
>> +    size_t pos = transactions()->find(transaction);
> 
> You should consider storing transactions() in a local variable to avoid hit the mutex so often (especially in the loop below, etc).

Done.

>> Source/WebCore/storage/IDBPendingTransactionMonitor.cpp:59
>> +        transactions()->at(i)->abort();
> 
> Shouldn't this also remove all of the items in addition to aborting them (transactions().clear())?

Yes.  Thanks for catching that.

[David Grogan]

Created attachment 117113 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 117113 [details]
Patch for landing

Rejecting attachment 117113 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
e
Source/WebCore/storage/IDBPendingTransactionMonitor.cpp:59: error: expected primary-expression before '>' token
Source/WebCore/storage/IDBPendingTransactionMonitor.cpp:59: error: 'transactionList' was not declared in this scope
Source/WebCore/storage/IDBPendingTransactionMonitor.cpp:59: error: 'transactions' was not declared in this scope
make: *** [out/Release/obj.target/webcore_remaining/Source/WebCore/storage/IDBPendingTransactionMonitor.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/10709161

[WebKit Review Bot]

Comment on attachment 117113 [details]
Patch for landing

Attachment 117113 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10694169

[David Grogan]

Created attachment 117229 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117229 [details]
Patch

Attachment 117229 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10704436

[David Grogan]

Created attachment 117344 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 117344 [details]
Patch for landing

Clearing flags on attachment: 117344

Committed r101626: <http://trac.webkit.org/changeset/101626>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.