Bug 144000 - Don't de-allocate FunctionRareData
Summary: Don't de-allocate FunctionRareData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 143999
Blocks: 144001
  Show dependency treegraph
 
Reported: 2015-04-21 10:53 PDT by Basile Clement
Modified: 2015-04-22 10:41 PDT (History)
4 users (show)

See Also:


Attachments
The patch (13.35 KB, patch)
2015-04-21 14:23 PDT, Basile Clement
msaboff: review-
Details | Formatted Diff | Diff
Addressed msaboff's remarks (12.72 KB, patch)
2015-04-21 17:35 PDT, Basile Clement
msaboff: review+
msaboff: commit-queue-
Details | Formatted Diff | Diff
Remove needless ASSERT (12.69 KB, patch)
2015-04-21 17:50 PDT, Basile Clement
no flags Details | Formatted Diff | Diff
Hopefully correct patch (12.29 KB, patch)
2015-04-21 18:47 PDT, Basile Clement
no flags Details | Formatted Diff | Diff
Final patch (12.26 KB, patch)
2015-04-21 18:55 PDT, Basile Clement
no flags Details | Formatted Diff | Diff
Hopefully well-formed... (12.26 KB, patch)
2015-04-21 19:12 PDT, Basile Clement
msaboff: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Rebased patch (again) (12.06 KB, patch)
2015-04-22 09:49 PDT, Basile Clement
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basile Clement 2015-04-21 10:53:52 PDT
This allow to keep using the same allocationProfileWatchpointSet across recompilations, patch forthcoming.
Comment 1 Basile Clement 2015-04-21 14:23:14 PDT
Created attachment 251263 [details]
The patch
Comment 2 Michael Saboff 2015-04-21 15:51:37 PDT
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 3 Basile Clement 2015-04-21 16:04:09 PDT
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?
Comment 4 Basile Clement 2015-04-21 17:35:23 PDT
Created attachment 251278 [details]
Addressed msaboff's remarks
Comment 5 Michael Saboff 2015-04-21 17:41:40 PDT
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.
Comment 6 Basile Clement 2015-04-21 17:50:46 PDT
Created attachment 251281 [details]
Remove needless ASSERT
Comment 7 Basile Clement 2015-04-21 18:47:26 PDT
Created attachment 251287 [details]
Hopefully correct patch
Comment 8 Basile Clement 2015-04-21 18:55:08 PDT
Comment on attachment 251287 [details]
Hopefully correct patch

Oops, put the spurious ASSERT in again.
Comment 9 Basile Clement 2015-04-21 18:55:28 PDT
Created attachment 251289 [details]
Final patch
Comment 10 Filip Pizlo 2015-04-21 19:06:45 PDT
You should rebase this patch :-)
Comment 11 Basile Clement 2015-04-21 19:12:45 PDT
Created attachment 251291 [details]
Hopefully well-formed...
Comment 12 Filip Pizlo 2015-04-21 20:43:59 PDT
Comment on attachment 251291 [details]
Hopefully well-formed...

so, ByteCodeParser was already right?
Comment 13 Basile Clement 2015-04-21 21:38:48 PDT
(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 .
Comment 14 Basile Clement 2015-04-21 21:41:08 PDT
(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 15 Michael Saboff 2015-04-22 09:30:01 PDT
Comment on attachment 251291 [details]
Hopefully well-formed...

r=me
Comment 16 WebKit Commit Bot 2015-04-22 09:33:05 PDT
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
Comment 17 Basile Clement 2015-04-22 09:49:25 PDT
Created attachment 251332 [details]
Rebased patch (again)
Comment 18 WebKit Commit Bot 2015-04-22 10:41:07 PDT
Comment on attachment 251332 [details]
Rebased patch (again)

Clearing flags on attachment: 251332

Committed r183113: <http://trac.webkit.org/changeset/183113>
Comment 19 WebKit Commit Bot 2015-04-22 10:41:11 PDT
All reviewed patches have been landed.  Closing bug.