Bug 144599

Summary: Add a fast path for simple Weak finalization.
Product: WebKit Reporter: Andreas Kling <kling>
Component: JavaScriptCoreAssignee: Andreas Kling <kling>
Status: NEW    
Severity: Normal CC: ggaren
Priority: P2 Keywords: Performance
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch idea none

Andreas Kling
Reported 2015-05-04 15:08:14 PDT
Hack from weekend: fast path for finalizing weaks that really just want to clear out a Weak.
Attachments
Patch idea (8.50 KB, patch)
2015-05-04 15:09 PDT, Andreas Kling
no flags
Andreas Kling
Comment 1 2015-05-04 15:09:06 PDT
Created attachment 252338 [details] Patch idea
Geoffrey Garen
Comment 2 2015-05-04 19:37:11 PDT
Comment on attachment 252338 [details] Patch idea View in context: https://bugs.webkit.org/attachment.cgi?id=252338&action=review > Source/JavaScriptCore/heap/WeakImpl.h:48 > + enum FinalizationMode { enum class please. > Source/JavaScriptCore/heap/WeakSetInlines.h:51 > + WeakImpl** impl = static_cast<WeakImpl**>(weakImpl->context()); WeakImpl* would be an "impl". This is something else. I usually follow the Qt-ism, and call variables like this "slot". > Source/WebCore/bindings/js/ScriptWrappableInlines.h:49 > + m_wrapper = JSC::Weak<JSDOMWrapper>(wrapper, wrapperOwner, &m_wrapper, false); I don't like ", false" as the indicator for this substantial change in behavior. :( Perhaps a better API would use a separate constructor, which took a WeakImpl** as an argument, signifying that you wanted the simple behavior by passing in the slot to clear. Otherwise, an enum tag should work. It's very scary to, on the one hand, say that you want the simple behavior and, on the other hand, pass in an owner that has a finalizer with non-simple behavior. What if someone adds some code to that finalizer and expects it to run? Can we arrange to separate out a base class owner, which has no finalizer, from a subclass owner, which has one?
Andreas Kling
Comment 3 2015-05-04 20:51:26 PDT
Comment on attachment 252338 [details] Patch idea Back to drawing board to address Geoff's comments.
Note You need to log in before you can comment on or make changes to this bug.