Bug 153792

Created attachment 382690 [details] Patch

Created attachment 382691 [details] Patch

Created attachment 382692 [details] Patch

Created attachment 382695 [details] Patch

Created attachment 382696 [details] Patch

Created attachment 382699 [details] Patch

Created attachment 382700 [details] Patch

Created attachment 382704 [details] Patch

Created attachment 382705 [details] Patch

Created attachment 382703 [details] Patch

Created attachment 382706 [details] Patch

Created attachment 382711 [details] Patch

Created attachment 382713 [details] Patch

Comment on attachment 382713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382713&action=review r=me with a few comments > Source/JavaScriptCore/bytecode/CodeBlock.cpp:903 > + case SourceCodeRepresentation::LinkTimeConstant: maybe we should have global object in the name of this enum value since it's dependent on global object? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:838 > + , m_linkTimeConstantRegisters(numberOfLinkTimeConstants, nullptr) This seems wasteful of memory. Any given compile should only have a few of these at most on average. Why not just use a HashMap instead of this Vector? (If we keep the vector, we might as well make it have this as the inline capacity since it's statically known exactly how big the vector will be. But I think HashMap is better.) > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:467 > +macro loadConstant(size, index, tag, payload) can other code that branches on if it's a constant use this helper (below in the LLInt)? > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:469 > +macro loadConstant(size, index, value) can the code below use this as a helper? > Source/JavaScriptCore/runtime/JSGlobalObject.h:423 > + Vector<LazyProperty<JSGlobalObject, JSCell>> m_linkTimeConstants; why not have the correct inline capacity? Or do we really want this out of line? > Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h:113 > +inline JSFunction* JSGlobalObject::throwTypeErrorFunction() const { return bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::throwTypeErrorFunction)); } > +inline JSFunction* JSGlobalObject::newPromiseCapabilityFunction() const { return bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::newPromiseCapability)); } > +inline JSFunction* JSGlobalObject::resolvePromiseFunction() const { return bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::resolvePromise)); } > +inline JSFunction* JSGlobalObject::rejectPromiseFunction() const { return bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::rejectPromise)); } > +inline JSFunction* JSGlobalObject::promiseProtoThenFunction() const { return bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::defaultPromiseThen)); } > +inline JSFunction* JSGlobalObject::regExpProtoExecFunction() const { return bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::regExpBuiltinExec)); } > +inline GetterSetter* JSGlobalObject::regExpProtoGlobalGetter() const { return bitwise_cast<GetterSetter*>(linkTimeConstant(LinkTimeConstant::regExpProtoGlobalGetter)); } > +inline GetterSetter* JSGlobalObject::regExpProtoUnicodeGetter() const { return bitwise_cast<GetterSetter*>(linkTimeConstant(LinkTimeConstant::regExpProtoUnicodeGetter)); } why not static cast for all of these? > LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value-expected.txt:17 > + this: c - Why has code you have in this patch changed the backtrace?

Comment on attachment 382713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382713&action=review Thanks! >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:903 >> + case SourceCodeRepresentation::LinkTimeConstant: > > maybe we should have global object in the name of this enum value since it's dependent on global object? Talked with Saam. For now, I'll use LinkTimeConstant since it represents constant populated at linking time. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:838 >> + , m_linkTimeConstantRegisters(numberOfLinkTimeConstants, nullptr) > > This seems wasteful of memory. Any given compile should only have a few of these at most on average. Why not just use a HashMap instead of this Vector? > (If we keep the vector, we might as well make it have this as the inline capacity since it's statically known exactly how big the vector will be. But I think HashMap is better.) Right. Changed this to HashMap. >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:467 >> +macro loadConstant(size, index, tag, payload) > > can other code that branches on if it's a constant use this helper (below in the LLInt)? Fixed. >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:469 >> +macro loadConstant(size, index, value) > > can the code below use this as a helper? Fixed. >> Source/JavaScriptCore/runtime/JSGlobalObject.h:423 >> + Vector<LazyProperty<JSGlobalObject, JSCell>> m_linkTimeConstants; > > why not have the correct inline capacity? Or do we really want this out of line? We cannot get this information in JSGlobalObject.h since it is included and used before generating JSBuiltins.h. We should not use LinkTimeConstant's JSBuiltins.h information here. >> Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h:113 >> +inline GetterSetter* JSGlobalObject::regExpProtoUnicodeGetter() const { return bitwise_cast<GetterSetter*>(linkTimeConstant(LinkTimeConstant::regExpProtoUnicodeGetter)); } > > why not static cast for all of these? I use jsCast for JSFunction while I continue using bitwise_cast for GetterSetter. Using static_cast / jsCast requires complete type declaration of GetterSetter here, but GetterSetter.h is not exposed outside of JSC. >> LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value-expected.txt:17 >> + this: c - > > Why has code you have in this patch changed the backtrace? I think this part is not dumping meaningful value before this change. If you change "c" to "d" in this test, this value becomes "test". And "test" as thisValue seems also incorrect. My guess is that this is related to hash table's value. But anyway, I'll file a bug and ask to inspector folks.

Committed r252032: <https://trac.webkit.org/changeset/252032>

<rdar://problem/56890056>

I guess Windows build failure is due to broken forwarding header thing

(In reply to Yusuke Suzuki from comment #18) > I guess Windows build failure is due to broken forwarding header thing Yes, seems like it is.