Summary: | Improve error message for uninitialized |this| in derived constructor | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Shvayka <ashvayka> | ||||||
Component: | JavaScriptCore | Assignee: | Alexey Shvayka <ashvayka> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Enhancement | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Alexey Shvayka
2021-01-01 11:50:42 PST
Created attachment 416882 [details]
Patch
Comment on attachment 416882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416882&action=review We found that our bytecode generation is too generic, and emitting many bytecodes. This prevents us from performing inlining in https://bugs.webkit.org/show_bug.cgi?id=220204. So, can you change this without increasing bytecode size? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2852 > +void BytecodeGenerator::emitTDZCheckForThisRegisterInDerivedConstructor() > +{ > + Ref<Label> thisIsNotInTDZ = newLabel(); > + emitJumpIfFalse(emitIsEmpty(newTemporary(), thisRegister()), thisIsNotInTDZ.get()); > + emitThrowReferenceError("'super()' must be called in derived constructor before accessing |this| or returning non-object."_s); > + emitLabel(thisIsNotInTDZ.get()); The problem is that, Comment on attachment 416882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416882&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2852 >> + emitLabel(thisIsNotInTDZ.get()); > > The problem is that, The problem is that, previously, it was check_tdz. But now, it is more bytecodes. This will cause performance/memory-usage problem because it increases bytecode size. I think that, 1. Since this is super rare case in practice, we should not emit bytecodes much for that pattern. 2. I think we should switch to the different solution: we should throw appropriate error message ad-hocly when we throw an error. Like, when we failed with check_tdz, we should investigate what causes it by checking bytecode stream / source code etc. and throws appropriate error. (In reply to Yusuke Suzuki from comment #3) > The problem is that, previously, it was check_tdz. But now, it is more > bytecodes. This will cause performance/memory-usage problem because it > increases bytecode size. Yes, there is an increase in bytecode size, but only for derived constructors, which are super rare. (In reply to Alexey Shvayka from comment #4) > (In reply to Yusuke Suzuki from comment #3) > > The problem is that, previously, it was check_tdz. But now, it is more > > bytecodes. This will cause performance/memory-usage problem because it > > increases bytecode size. > > Yes, there is an increase in bytecode size, but only for derived > constructors, which are super rare. Please see BytecodeGenerator::ensureThis(): emitTDZCheckForThisRegisterInDerivedConstructor(), that emits additional bytecodes, is guarded by `if (constructorKind() == ConstructorKind::Extends || isDerivedConstructorContext())`. (In reply to Alexey Shvayka from comment #5) > Please see BytecodeGenerator::ensureThis(): > emitTDZCheckForThisRegisterInDerivedConstructor(), that emits additional > bytecodes, is guarded by `if (constructorKind() == ConstructorKind::Extends > || isDerivedConstructorContext())`. Discussed this on Slack: we should avoid having super rare things, like TDZ errors, in bytecode, and use slow paths instead. Emitting many bytecodes for error checking can penalize CodeBlock heuristics, telling DFG / FTL that the function is too large. Created attachment 416887 [details]
Patch
Comment on attachment 416887 [details]
Patch
r=me
Committed r271120: <https://trac.webkit.org/changeset/271120> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416887 [details]. |