...
<rdar://problem/59175669>
Created attachment 393467 [details] Patch Add tests later
Created attachment 393468 [details] Patch Add tests later
Created attachment 393574 [details] Patch
Created attachment 393579 [details] Patch
Comment on attachment 393579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393579&action=review r=me with one concern about JSAPIValueWrapper on !CPU(ADDRESS64) targets. > Source/JavaScriptCore/API/MarkedJSValueRefArray.cpp:60 > + visitor.appendUnbarriered(jsCell); // We should mark the wrapper itself to keep JSValueRef live. I didn't quite understand what this comment meant in the first place until I read JSC::JSValue toJS(JSC::JSGlobalObject* globalObject, JSValueRef v). From my reading of the code, I see that the jsCell here may be a JSAPIValueWrapper which is itself a JSCell. Hence, we should visit the JSAPIValueWrapper itself, and that means appending the jsCell here should do the right thing regardless of whether the jsCell is a JSAPIValueWrapper or some other JSCell. That said, JSAPIValueWrapper does not appear to have a visitChildren() function. Hence, I don't see how its m_value will get visited. Am I missing something? If the above is accurate, I think we don't need this comment. We do need to look into how the JSAPIValueWrapper::m_value gets visited, and add a visitChildren() there for the !CPU(ADDRESS64) case. Should probably do that in a separate bug. > Source/JavaScriptCore/heap/Heap.cpp:3011 > +void Heap::addmarkedJSValueRefArray(MarkedJSValueRefArray* array) typo: /addmarkedJSValueRefArray/addMarkedJSValueRefArray/
Comment on attachment 393579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393579&action=review >> Source/JavaScriptCore/API/MarkedJSValueRefArray.cpp:60 >> + visitor.appendUnbarriered(jsCell); // We should mark the wrapper itself to keep JSValueRef live. > > I didn't quite understand what this comment meant in the first place until I read JSC::JSValue toJS(JSC::JSGlobalObject* globalObject, JSValueRef v). > > From my reading of the code, I see that the jsCell here may be a JSAPIValueWrapper which is itself a JSCell. Hence, we should visit the JSAPIValueWrapper itself, and that means appending the jsCell here should do the right thing regardless of whether the jsCell is a JSAPIValueWrapper or some other JSCell. > > That said, JSAPIValueWrapper does not appear to have a visitChildren() function. Hence, I don't see how its m_value will get visited. Am I missing something? > > If the above is accurate, I think we don't need this comment. We do need to look into how the JSAPIValueWrapper::m_value gets visited, and add a visitChildren() there for the !CPU(ADDRESS64) case. Should probably do that in a separate bug. JSAPIValueWrapper is only used for non-cell values (undefined, number etc.). So this means that the wrapped value itself does not require visitChildren. This wrapper exists only because we need to keep sizeof(JSValueRef) 32bit in 32bit architecture. Since our non-cell values require 64bit value representation, we decided to use JSAPIValueWrapper as a work-around for this problem.
Created attachment 393610 [details] Patch Patch for landing
Comment on attachment 393579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393579&action=review >> Source/JavaScriptCore/heap/Heap.cpp:3011 >> +void Heap::addmarkedJSValueRefArray(MarkedJSValueRefArray* array) > > typo: /addmarkedJSValueRefArray/addMarkedJSValueRefArray/ Fixed.
Committed r258478: <https://trac.webkit.org/changeset/258478>
Comment on attachment 393610 [details] Patch WTF::Vector with inline capacity was originally invented to use in local variables to create a safer alternative to variable length arrays that spills out onto the heap when needed, but has similar efficiency when the array is smaller than a fixed predicted size. I’m maybe a tiny bit surprised that we didn’t switch to use them in any of these cases.
Comment on attachment 393610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393610&action=review > Source/JavaScriptCore/API/JSContext.mm:237 > + [arguments setObject:[JSValue valueWithJSValueRef:entry->arguments[i] inContext:context] atIndexedSubscript:i]; This should just be calling addObject: rather than setObject:atIndexedSubscript: unless I am missing something. What’s the benefit of using setObject:atIndexedSubscript: here? > Source/JavaScriptCore/API/MarkedJSValueRefArray.h:68 > + unsigned m_size; > + JSValueRef m_inlineBuffer[inlineCapacity] { }; > + BufferUniquePtr m_buffer; Frustrating that this is basically re-implementing Vector with inline capacity. Is there some way to take advantage of Vector or are there just not enough template arguments? > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:396 > + Vector<GCGLint> params(numValues); Could use inline capacity here if we want to avoid allocating on the heap when there are small numbers of arguments. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:426 > + Vector<GCGLint> params(numValues); Ditto. > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:170 > + Vector<AudioObjectID> deviceIDs(deviceCount); Ditto. > Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:65 > + Vector<CGDirectDisplayID> activeDisplays(displayCount); Ditto. > Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:297 > + Vector<CGPoint> locations(touchCount); Ditto. > Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:330 > + Vector<CGPoint> locations(touchCount); Ditto. > Source/WebKit/UIProcess/_WKTouchEventGenerator.mm:348 > + Vector<CGPoint> startLocations(touchCount); > + Vector<CGPoint> nextLocations(touchCount); Ditto.