Bug 144779

Summary: Clean up some possible RefPtr to PassRefPtr churn
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, joepeck
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[PATCH] Proposed Fix
darin: review+, darin: commit-queue-
[PATCH] For Bots
none
[PATCH] For Bots none

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>