WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
114529
HeapTimer lifetime should be less complicated
https://bugs.webkit.org/show_bug.cgi?id=114529
Summary
HeapTimer lifetime should be less complicated
Mark Hahnenberg
Reported
2013-04-12 13:21:23 PDT
Right now our HeapTimer lifetime is rather complicated. HeapTimers are "owned" by the JSGlobalData, but there's an issue in that there can be races between a thread that is trying to tear down a JSGlobalData and the HeapTimer's fire function. Our current code for tearing down HeapTimers is an intricate and delicate dance which probably contains subtle bugs. We can make our lives easier by changing things around a bit. (1) We should free the API lock from being solely owned by the JSGlobalData so we don't have to worry about grabbing the lock out of invalid memory when our HeapTimer callback fires. (2) We should also make it so that we deref the JSGlobalData first, then unlock the API lock so that when we have the lock, the JSGlobalData is in one of two states: fully valid or completely destroyed, and we know exactly which one. (3) The JSLock can tell us this information by keeping a back pointer to the JSGlobalData. When the JSGlobalData's destructor is called, it clears this pointer in the JSLock. Other clients of the API lock can then check this pointer to determine whether or not the JSGlobalData is still around. (4) The CFRunLoopTimer will use the API lock as its context rather than the HeapTimer itself. The only way the HeapTimer's callback can get to the HeapTimer is through the API lock's JSGlobalData pointer. (5) The CFRunLoopTimerContext struct has two fields for retain and release callbacks for the context's info field. We'll provide these callbacks to ref() and deref() the JSLock as necessary. Thus, the timer becomes the other owner of the JSLock apart from the JSGlobalData.
Attachments
Patch
(18.88 KB, patch)
2013-04-12 13:39 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Patch
(23.61 KB, patch)
2013-04-12 17:12 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Patch
(23.63 KB, patch)
2013-04-13 16:36 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Patch
(24.44 KB, patch)
2013-04-15 09:54 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Patch
(24.49 KB, patch)
2013-04-15 13:45 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Mark Hahnenberg
Comment 1
2013-04-12 13:39:08 PDT
Created
attachment 197880
[details]
Patch
Darin Adler
Comment 2
2013-04-12 13:51:11 PDT
Comment on
attachment 197880
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=197880&action=review
review- because I think you’ll want to make at least one of the changes I suggest
> Source/JavaScriptCore/jsc.cpp:765 > - RefPtr<JSGlobalData> globalData = JSGlobalData::create(LargeHeap); > - JSLockHolder lock(globalData.get()); > + JSGlobalData* globalData = JSGlobalData::create(LargeHeap).leakRef(); > + APIEntryShim shim(globalData);
I don’t understand this change. Change log does not make it clear. Per-function comments can sometimes help with this.
> Source/JavaScriptCore/testRegExp.cpp:504 > - RefPtr<JSGlobalData> globalData = JSGlobalData::create(LargeHeap); > - JSLockHolder lock(globalData.get()); > + JSGlobalData* globalData = JSGlobalData::create(LargeHeap).leakRef(); > + APIEntryShim shim(globalData);
I don’t understand this change.
> Source/JavaScriptCore/heap/Heap.cpp:266 > + , m_activityCallback(adoptPtr(DefaultGCActivityCallback::create(this))) > + , m_sweeper(adoptPtr(IncrementalSweeper::create(this)))
Instead of changing these call sites, the return types of the create functions should be changed to PassOwnPtr. We want the calls to adoptPtr to be right where the call to new is, not somewhere else.
> Source/JavaScriptCore/heap/Heap.cpp:816 > void Heap::setActivityCallback(GCActivityCallback* activityCallback) > { > - m_activityCallback = activityCallback; > + m_activityCallback = adoptPtr(activityCallback); > }
The argument type of setActivityCallback should be changed to PassOwnPtr. We want the calls to adoptPtr to be right where the call to new is, not somewhere else.
> Source/JavaScriptCore/heap/HeapTimer.cpp:56 > + m_context.retain = &retainAPILock; > + m_context.release = &releaseAPILock;
I don’t think these & operators are needed, and I normally prefer to omit them.
> Source/JavaScriptCore/heap/HeapTimer.h:29 > +#include "JSLock.h"
Why is this include needed? I don’t see any changes to the header below that seem to require it.
> Source/JavaScriptCore/heap/HeapTimer.h:73 > + static const void* retainAPILock(const void*); > + static void releaseAPILock(const void*);
Why do these need to be member functions? Can’t they just be free functions, private to the cpp file?
Mark Hahnenberg
Comment 3
2013-04-12 17:12:54 PDT
Created
attachment 197912
[details]
Patch
Early Warning System Bot
Comment 4
2013-04-12 17:26:31 PDT
Comment on
attachment 197912
[details]
Patch
Attachment 197912
[details]
did not pass qt-ews (qt): Output:
http://webkit-queues.appspot.com/results/107068
Early Warning System Bot
Comment 5
2013-04-12 17:27:53 PDT
Comment on
attachment 197912
[details]
Patch
Attachment 197912
[details]
did not pass qt-wk2-ews (qt): Output:
http://webkit-queues.appspot.com/results/96075
Build Bot
Comment 6
2013-04-13 06:25:36 PDT
Comment on
attachment 197912
[details]
Patch
Attachment 197912
[details]
did not pass win-ews (win): Output:
http://webkit-queues.appspot.com/results/97134
Mark Hahnenberg
Comment 7
2013-04-13 16:36:51 PDT
Created
attachment 197962
[details]
Patch
Early Warning System Bot
Comment 8
2013-04-13 16:45:26 PDT
Comment on
attachment 197962
[details]
Patch
Attachment 197962
[details]
did not pass qt-ews (qt): Output:
http://webkit-queues.appspot.com/results/95332
Early Warning System Bot
Comment 9
2013-04-14 01:47:53 PDT
Comment on
attachment 197962
[details]
Patch
Attachment 197962
[details]
did not pass qt-wk2-ews (qt): Output:
http://webkit-queues.appspot.com/results/154081
Mark Hahnenberg
Comment 10
2013-04-15 09:54:37 PDT
Created
attachment 198131
[details]
Patch
Oliver Hunt
Comment 11
2013-04-15 10:54:40 PDT
Comment on
attachment 198131
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=198131&action=review
> Source/JavaScriptCore/testRegExp.cpp:504 > - RefPtr<JSGlobalData> globalData = JSGlobalData::create(LargeHeap); > - JSLockHolder lock(globalData.get()); > + JSGlobalData* globalData = JSGlobalData::create(LargeHeap).leakRef(); > + APIEntryShim shim(globalData);
Why can't we use a refptr here anymore?
> Source/JavaScriptCore/API/APIShims.h:55 > JSGlobalData* m_globalData;
Just make m_globalData a RefPtr<> now
> Source/JavaScriptCore/API/APIShims.h:79 > + RefPtr<JSLock> apiLock = &m_globalData->apiLock(); > + m_globalData->deref(); > + apiLock->unlock();
Just make APIEntryShim have a JSLockHolder to hold on to apiLock, if it's still strictly necessary you can just do m_globalData.clear()
Mark Hahnenberg
Comment 12
2013-04-15 11:05:58 PDT
Comment on
attachment 198131
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=198131&action=review
>> Source/JavaScriptCore/testRegExp.cpp:504 >> + APIEntryShim shim(globalData); > > Why can't we use a refptr here anymore?
Because we need to hold the lock when we destroy a JSGlobalData, but we destroy the JSLockHolder first (which releases the lock), then the RefPtr (which destroys the JSGlobalData).
Mark Hahnenberg
Comment 13
2013-04-15 11:08:16 PDT
> Because we need to hold the lock when we destroy a JSGlobalData, but we destroy the JSLockHolder first (which releases the lock), then the RefPtr (which destroys the JSGlobalData).
I guess we could continue to use a RefPtr, but explicitly clear it prior to releasing the API lock.
Oliver Hunt
Comment 14
2013-04-15 12:07:00 PDT
(In reply to
comment #12
)
> (From update of
attachment 198131
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=198131&action=review
> > >> Source/JavaScriptCore/testRegExp.cpp:504 > >> + APIEntryShim shim(globalData); > > > > Why can't we use a refptr here anymore? > > Because we need to hold the lock when we destroy a JSGlobalData, but we destroy the JSLockHolder first (which releases the lock), then the RefPtr (which destroys the JSGlobalData).
What happens if we just make the api lock object refcounted, the heap can reference it, the timer can reference it, etc, etc. Then the lock automatically becomes the last thing to die
Mark Hahnenberg
Comment 15
2013-04-15 13:45:46 PDT
Created
attachment 198180
[details]
Patch
WebKit Commit Bot
Comment 16
2013-04-15 16:16:33 PDT
Comment on
attachment 198180
[details]
Patch Clearing flags on attachment: 198180 Committed
r148475
: <
http://trac.webkit.org/changeset/148475
>
WebKit Commit Bot
Comment 17
2013-04-15 16:16:35 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug