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 tree / graph
 
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
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

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.