Summary: | Don't de-allocate FunctionRareData | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Basile Clement <basile_clement> | ||||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Enhancement | CC: | commit-queue, fpizlo, ggaren, msaboff | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 143999 | ||||||||||||||||||
Bug Blocks: | 144001 | ||||||||||||||||||
Attachments: |
|
Description
Basile Clement
2015-04-21 10:53:52 PDT
Created attachment 251263 [details]
The patch
Comment on attachment 251263 [details] The patch View in context: https://bugs.webkit.org/attachment.cgi?id=251263&action=review r-. Almost there. > Source/JavaScriptCore/ChangeLog:7 > + Please provide why you made this change and a summary of what you did. > Source/JavaScriptCore/runtime/FunctionRareData.cpp:-82 > - Base::finishCreation(vm); Where do we call the super class finishCreation? > Source/JavaScriptCore/runtime/JSFunction.cpp:113 > + VM& vm = exec->vm(); Add an ASSERT that m_rareData is not set. > Source/JavaScriptCore/runtime/JSFunction.cpp:401 > + thisObject->m_rareData->allocationProfile()->clear(); > thisObject->m_rareData->allocationProfileWatchpointSet().fireAll("Store to prototype property of a function"); Encapsulate this code in a FunctionRareData::clear() or FunctionRareData::clearAllocationProfile() method. > Source/JavaScriptCore/runtime/JSFunction.cpp:450 > + thisObject->m_rareData->allocationProfile()->clear(); > thisObject->m_rareData->allocationProfileWatchpointSet().fireAll("Store to prototype property of a function"); Ditto. Comment on attachment 251263 [details] The patch View in context: https://bugs.webkit.org/attachment.cgi?id=251263&action=review >> Source/JavaScriptCore/runtime/FunctionRareData.cpp:-82 >> - Base::finishCreation(vm); > > Where do we call the super class finishCreation? FunctionRareData no longer has a finishCreation member, so that is done by rareData->finishCreation(vm) in the FunctionRareData::create() call. Is this bad style without the using Base::finishCreation; clause? Created attachment 251278 [details]
Addressed msaboff's remarks
Comment on attachment 251278 [details] Addressed msaboff's remarks View in context: https://bugs.webkit.org/attachment.cgi?id=251278&action=review r=me with one minor nit. Repost with the fix so I can cq+ > Source/JavaScriptCore/runtime/JSFunction.cpp:120 > + ASSERT(!!m_rareData); This ASSERT is not needed. Created attachment 251281 [details]
Remove needless ASSERT
Created attachment 251287 [details]
Hopefully correct patch
Comment on attachment 251287 [details]
Hopefully correct patch
Oops, put the spurious ASSERT in again.
Created attachment 251289 [details]
Final patch
You should rebase this patch :-) Created attachment 251291 [details]
Hopefully well-formed...
Comment on attachment 251291 [details]
Hopefully well-formed...
so, ByteCodeParser was already right?
(In reply to comment #12) > Comment on attachment 251291 [details] > Hopefully well-formed... > > so, ByteCodeParser was already right? This change should be transparent for ByteCodeParser. It is already handling the case where the allocation structure may be empty from https://bugs.webkit.org/show_bug.cgi?id=143999 . (In reply to comment #13) > (In reply to comment #12) > > Comment on attachment 251291 [details] > > Hopefully well-formed... > > > > so, ByteCodeParser was already right? > > This change should be transparent for ByteCodeParser. > It is already handling the case where the allocation structure may be empty > from https://bugs.webkit.org/show_bug.cgi?id=143999 . I do agree that the corresponding changes have probably been put on the wrong patch however... Comment on attachment 251291 [details]
Hopefully well-formed...
r=me
Comment on attachment 251291 [details] Hopefully well-formed... Rejecting attachment 251291 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 251291, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: /LowLevelInterpreter32_64.asm patching file Source/JavaScriptCore/llint/LowLevelInterpreter64.asm patching file Source/JavaScriptCore/runtime/FunctionRareData.cpp patching file Source/JavaScriptCore/runtime/FunctionRareData.h patching file Source/JavaScriptCore/runtime/JSFunction.cpp patching file Source/JavaScriptCore/runtime/JSFunction.h Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Michael Saboff']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/5617147334098944 Created attachment 251332 [details]
Rebased patch (again)
Comment on attachment 251332 [details] Rebased patch (again) Clearing flags on attachment: 251332 Committed r183113: <http://trac.webkit.org/changeset/183113> All reviewed patches have been landed. Closing bug. |