Bug 144000 – Don't de-allocate FunctionRareData

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Basile Clement]

This allow to keep using the same allocationProfileWatchpointSet across recompilations, patch forthcoming.

[Basile Clement]

Created attachment 251263 [details]
The patch

[Michael Saboff]

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.

[Basile Clement]

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?

[Basile Clement]

Created attachment 251278 [details]
Addressed msaboff's remarks

[Michael Saboff]

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.

[Basile Clement]

Created attachment 251281 [details]
Remove needless ASSERT

[Basile Clement]

Created attachment 251287 [details]
Hopefully correct patch

[Basile Clement]

Comment on attachment 251287 [details]
Hopefully correct patch

Oops, put the spurious ASSERT in again.

[Basile Clement]

Created attachment 251289 [details]
Final patch

[Filip Pizlo]

You should rebase this patch :-)

[Basile Clement]

Created attachment 251291 [details]
Hopefully well-formed...

[Filip Pizlo]

Comment on attachment 251291 [details]
Hopefully well-formed...

so, ByteCodeParser was already right?

[Basile Clement]

(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 .

[Basile Clement]

(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...

[Michael Saboff]

Comment on attachment 251291 [details]
Hopefully well-formed...

r=me

[WebKit Commit Bot]

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

[Basile Clement]

Created attachment 251332 [details]
Rebased patch (again)

[WebKit Commit Bot]

Comment on attachment 251332 [details]
Rebased patch (again)

Clearing flags on attachment: 251332

Committed r183113: <http://trac.webkit.org/changeset/183113>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.