We reify lazy property names in a few different ways, each specific to the JSCell implementation, in put() instead of having a special function to do reification. Let's make that simpler.
<rdar://problem/35492521>
Created attachment 326996 [details] patch
Attachment 326996 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSFunction.h:193: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 326996 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=326996&action=review > Source/JavaScriptCore/runtime/JSObjectInlines.h:267 > + switch (methodTable(vm)->reifyPropertyNameIfNeeded(this, globalObject(vm)->globalExec(), propertyName)) { > + case PropertyNameReified::Nothing: break; > + case PropertyNameReified::Something: break; > + case PropertyNameReified::TriedButFailed: RELEASE_ASSERT_NOT_REACHED(); > + } Though I like this style for conciseness, I think the WebKit way is to have the break statement and assert on a different line.
Comment on attachment 326996 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=326996&action=review > Source/JavaScriptCore/runtime/ClassInfo.h:41 > - typedef void (*DestroyFunctionPtr)(JSCell*); > + using DestroyFunctionPtr = void (*)(JSCell*); I suggest doing these refactoring in a subsequent patch. It is actually not essential to this patch. > Source/JavaScriptCore/runtime/JSCell.h:63 > +enum class PropertyNameReified { nit: I think PropertyReificationResult is a better name. > Source/JavaScriptCore/runtime/JSFunction.h:192 > + enum class LazyProperty I think a better name for this is "Laziness" or ""LazinessState". "LazyProperty" implies a JS property which can add confusion. > Source/JavaScriptCore/runtime/JSFunction.h:196 > + IsLazyReified, I suggest simply naming this "IsReified". "IsLazyReified" has this ambiguity about whether it has been reified yet when you do mean that it is definitely reified. The fact that it is "reified" implies that it was once lazy. So, "IsReified" is sufficient and clearer IMHO. >> Source/JavaScriptCore/runtime/JSObjectInlines.h:267 >> + switch (methodTable(vm)->reifyPropertyNameIfNeeded(this, globalObject(vm)->globalExec(), propertyName)) { >> + case PropertyNameReified::Nothing: break; >> + case PropertyNameReified::Something: break; >> + case PropertyNameReified::TriedButFailed: RELEASE_ASSERT_NOT_REACHED(); >> + } > > Though I like this style for conciseness, I think the WebKit way is to have the break statement and assert on a different line. Hmmm, one of the downside of this generic approach is that it burdens all objects with a reifyPropertyNameIfNeeded() check even when only a few sub-classes need it. Is it possible to avoid this overhead? Or is the overhead insignificant? If perf is an issue, maybe you can check a flag (e.g. HasLazilyReifiedProperties) before choosing to do the virtual dispatch. See use of the OverridesGetPropertyNames attribute.
Created attachment 327017 [details] patch Changes suggested by Mark. Will run release perf numbers to see if it's worth doing the last suggestion.
Attachment 327017 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSFunction.h:193: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 327073 [details] results Perf results are "might be 1.0035x slower". Nothing really stands out, seems to be in the noise.
Created attachment 327075 [details] patch Fix style nit.
Comment on attachment 327075 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=327075&action=review r=me. > Source/JavaScriptCore/runtime/JSFunction.h:204 > + enum class PropertyStatus { > + Eager, > + Lazy, > + Reified, > + }; > + static bool isLazy(PropertyStatus property) { return property == PropertyStatus::Lazy || property == PropertyStatus::Reified; } > + static bool isReified(PropertyStatus property) { return property == PropertyStatus::Reified; } > + > + PropertyStatus reifyLazyPropertyIfNeeded(VM&, ExecState*, PropertyName); > + PropertyStatus reifyLazyPropertyForHostOrBuiltinIfNeeded(VM&, ExecState*, PropertyName); > + PropertyStatus reifyLazyLengthIfNeeded(VM&, ExecState*, PropertyName); > + PropertyStatus reifyLazyNameIfNeeded(VM&, ExecState*, PropertyName); > + PropertyStatus reifyLazyBoundNameIfNeeded(VM&, ExecState*, PropertyName); It seems a little silly that we have all this in addition to the method table functionality. Can you file a bug to clean this up later?
(In reply to Keith Miller from comment #10) > Comment on attachment 327075 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=327075&action=review > > r=me. > > > Source/JavaScriptCore/runtime/JSFunction.h:204 > > + enum class PropertyStatus { > > + Eager, > > + Lazy, > > + Reified, > > + }; > > + static bool isLazy(PropertyStatus property) { return property == PropertyStatus::Lazy || property == PropertyStatus::Reified; } > > + static bool isReified(PropertyStatus property) { return property == PropertyStatus::Reified; } > > + > > + PropertyStatus reifyLazyPropertyIfNeeded(VM&, ExecState*, PropertyName); > > + PropertyStatus reifyLazyPropertyForHostOrBuiltinIfNeeded(VM&, ExecState*, PropertyName); > > + PropertyStatus reifyLazyLengthIfNeeded(VM&, ExecState*, PropertyName); > > + PropertyStatus reifyLazyNameIfNeeded(VM&, ExecState*, PropertyName); > > + PropertyStatus reifyLazyBoundNameIfNeeded(VM&, ExecState*, PropertyName); > > It seems a little silly that we have all this in addition to the method > table functionality. Can you file a bug to clean this up later? I filed radar 35592139 as a follow-up. I think we have a few other things that we can clean up!
Comment on attachment 327075 [details] patch Clearing flags on attachment: 327075 Committed r224927: <https://trac.webkit.org/changeset/224927>
All reviewed patches have been landed. Closing bug.
Comment on attachment 327075 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=327075&action=review > Source/JavaScriptCore/ChangeLog:24 > + We reify lazy property names in a few different ways, each > + specific to the JSCell implementation, in put() instead of having > + a special function to do reification. Let's make that simpler. > + > + This patch makes it easier to reify property names in a uniform > + manner, and does so in JSFunction. As a follow up I'll use the > + same mechanics for: > + > + ClonedArguments callee, iteratorSymbol (Symbol.iterator) > + ErrorConstructor stackTraceLimit > + ErrorInstance line, column, sourceURL, stack > + GenericArguments length, callee, iteratorSymbol (Symbol.iterator) > + GetterSetter RELEASE_ASSERT_NOT_REACHED() > + JSArray length > + RegExpObject lastIndex > + StringObject length I think if we truly did a generic form of lazy properties, it would mean we need to logically call your vtable function on get(). This would surely lead to slow downs. There are probably ways to make it work, bit it requires monitoring everyone that calls into the various property MethodTable hooks to make sure they first reifyLazyPropertyIfNeeded. This design feels untenable since we call into the method table in many places that are not just in JSObject. > Source/JavaScriptCore/runtime/JSCell.h:67 > +enum class PropertyReificationResult { > + Nothing, > + Something, > + TriedButFailed, // Sometimes the property name already exists but has special behavior and can't be reified, e.g. Array.length. > +}; I don't see a need to have this. > Source/JavaScriptCore/runtime/JSObjectInlines.h:267 > + switch (methodTable(vm)->reifyPropertyNameIfNeeded(this, globalObject(vm)->globalExec(), propertyName)) { > + case PropertyReificationResult::Nothing: break; > + case PropertyReificationResult::Something: break; > + case PropertyReificationResult::TriedButFailed: RELEASE_ASSERT_NOT_REACHED(); > + } This feels like the wrong place to have this to me. Until we have more evidence, I would have just done this inside put_getter_setter_by_id
*** Bug 157148 has been marked as a duplicate of this bug. ***