Bug 144779 - Clean up some possible RefPtr to PassRefPtr churn
Summary: Clean up some possible RefPtr to PassRefPtr churn
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-07 18:17 PDT by Joseph Pecoraro
Modified: 2016-09-07 21:42 PDT (History)
2 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (6.60 KB, patch)
2015-05-07 18:18 PDT, Joseph Pecoraro
darin: review+
darin: commit-queue-
Details | Formatted Diff | Diff
[PATCH] For Bots (7.09 KB, patch)
2015-05-13 12:33 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots (7.12 KB, patch)
2015-05-13 13:41 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-05-07 18:17:00 PDT
* SUMMARY
Clean up some possible RefPtr to PassRefPtr churn.

There are a few cases where the last use of a RefPtr can be releasing and isn't. This could likely converted to rvalue references, but for now just using release() should save a ref churn.
Comment 1 Joseph Pecoraro 2015-05-07 18:18:51 PDT
Created attachment 252670 [details]
[PATCH] Proposed Fix

Lets see what the bots think of this.
Comment 2 Joseph Pecoraro 2015-05-07 18:31:37 PDT
Comment on attachment 252670 [details]
[PATCH] Proposed Fix

LayoutTests/js tests passed for me locally.
Comment 3 Darin Adler 2015-05-10 15:15:38 PDT
Comment on attachment 252670 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=252670&action=review

Breaks the GTK build.

> Source/JavaScriptCore/ChangeLog:20
> +        Release the last use of a RefPtr as it is passed on.

Need to move these call sites to RefPtr or Ref instead of PassRefPtr: <http://www.webkit.org/coding/RefPtr.html>. Then the issue will still exist, but in far fewer cases because the compiler will do the move optimization since it can see the lifetimes.

> Source/JavaScriptCore/runtime/GenericTypedArrayViewInlines.h:70
> +    return adoptRef(new GenericTypedArrayView(buffer.release(), byteOffset, length));

platform/graphics/ISOVTTCue.cpp.o:ISOVTTCue.cpp:function WebCore::ISOBox::peekString(JSC::ArrayBuffer*, unsigned int, unsigned int): error: undefined reference to 'JSC::GenericTypedArrayView<JSC::Uint8Adaptor>::create(WTF::PassRefPtr<JSC::ArrayBuffer>, unsigned int, unsigned int)'
Comment 4 Joseph Pecoraro 2015-05-13 12:24:33 PDT
Comment on attachment 252670 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=252670&action=review

>> Source/JavaScriptCore/runtime/GenericTypedArrayViewInlines.h:70
>> +    return adoptRef(new GenericTypedArrayView(buffer.release(), byteOffset, length));
> 
> platform/graphics/ISOVTTCue.cpp.o:ISOVTTCue.cpp:function WebCore::ISOBox::peekString(JSC::ArrayBuffer*, unsigned int, unsigned int): error: undefined reference to 'JSC::GenericTypedArrayView<JSC::Uint8Adaptor>::create(WTF::PassRefPtr<JSC::ArrayBuffer>, unsigned int, unsigned int)'

Hmm, I don't understand the error. I'm not changing the signature of anything and the error messages don't mention an error inside the inline code, just that the top level method is not available.

Maybe "undefined reference to" might mean it doesn't have an include it needs to TypedArrayInlines? How would this have worked before?
Comment 5 Joseph Pecoraro 2015-05-13 12:33:08 PDT
Created attachment 253048 [details]
[PATCH] For Bots
Comment 6 Joseph Pecoraro 2015-05-13 13:41:06 PDT
Created attachment 253055 [details]
[PATCH] For Bots
Comment 7 Joseph Pecoraro 2015-05-13 17:43:29 PDT
Comment on attachment 253055 [details]
[PATCH] For Bots

Yay. Seems it might have just been missing includes.
Comment 8 WebKit Commit Bot 2015-05-13 18:35:36 PDT
Comment on attachment 253055 [details]
[PATCH] For Bots

Clearing flags on attachment: 253055

Committed r184325: <http://trac.webkit.org/changeset/184325>