The patch to be attached will refactor identifier resolution, as suggested in bug 75708 comment 2.
Created attachment 122450 [details] Patch
The attached patch is fairly large, but it shouldn't change anything semantically. It passes run-javascriptcore-tests with and without debug. What do you think, Geoffrey?
General direction looks good to me. One note for now, before a full review: you'll want to run run-webkit-tests for correctness testing, and bencher for performance testing, as well.
Created attachment 122631 [details] Patch
Comment on attachment 122631 [details] Patch This updated patch fixes a bug that run-webkit-tests caught, in which resolve() could return an invalid "type". The test results seem OK: 27935 tests ran as expected, 4 didn't: Unexpected flakiness: text diff mismatch (3) editing/pasteboard/pasting-empty-html-falls-back-to-text.html = TEXT PASS fast/forms/basic-buttons.html = TEXT PASS media/video-poster-blocked-by-willsendrequest.html = TEXT PASS Regressions: Unexpected image mismatch : (1) svg/foreignObject/repaint-rect-coordinates.html = IMAGE I'm assuming those problems have nothing to do with this patch.
Benchmark report for SunSpider, V8, and Kraken on \c. VMs tested: "trunk" at /home/wingo/src/WebKit/WebKitBuild/Release-trunk/Programs/jsc "refactor" at /home/wingo/src/WebKit/WebKitBuild/Release/Programs/jsc Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. trunk refactor SunSpider: 3d-cube 5.4096+-0.2538 5.1875+-0.2001 might be 1.0428x faster 3d-morph 7.6254+-0.3902 ? 7.6515+-0.2127 ? 3d-raytrace 8.1438+-0.6973 7.5884+-0.3058 might be 1.0732x faster access-binary-trees 1.7641+-0.1350 ? 1.7677+-0.1208 ? access-fannkuch 10.9894+-0.1674 10.8687+-0.1637 might be 1.0111x faster access-nbody 3.0872+-0.2183 ? 3.1068+-0.4445 ? access-nsieve 2.7876+-0.2191 ? 3.0385+-0.2362 ? might be 1.0900x slower bitops-3bit-bits-in-byte 1.5567+-0.2422 1.4197+-0.0994 might be 1.0966x faster bitops-bits-in-byte 2.4268+-0.1596 2.3921+-0.1263 might be 1.0145x faster bitops-bitwise-and 6.4219+-0.2260 6.4202+-0.3489 bitops-nsieve-bits 5.7165+-0.4385 5.4790+-0.1839 might be 1.0433x faster controlflow-recursive 2.0563+-0.1433 ? 2.2568+-0.3584 ? might be 1.0975x slower crypto-aes 6.5085+-0.3982 ? 6.5795+-0.3358 ? might be 1.0109x slower crypto-md5 2.9482+-0.6833 2.3396+-0.1538 might be 1.2601x faster crypto-sha1 2.2550+-0.3891 ? 2.4811+-0.3457 ? might be 1.1002x slower date-format-tofte 9.9916+-0.4122 ? 10.0765+-0.3077 ? date-format-xparb 8.6556+-0.2092 ? 8.7391+-0.3008 ? math-cordic 6.0482+-0.1594 6.0479+-0.1019 math-partial-sums 13.7263+-0.2892 ? 13.9744+-0.3798 ? might be 1.0181x slower math-spectral-norm 2.5806+-0.2523 2.5211+-0.1493 might be 1.0236x faster regexp-dna 7.8237+-0.2436 ? 7.8756+-0.3009 ? string-base64 5.1418+-0.3378 ? 5.3956+-0.2006 ? might be 1.0493x slower string-fasta 7.1104+-0.2327 ? 7.3781+-0.3737 ? might be 1.0376x slower string-tagcloud 10.8687+-0.4135 10.8683+-0.3032 string-unpack-code 17.9470+-0.2351 17.8377+-0.1830 string-validate-input 6.2236+-0.4266 6.1110+-0.4341 might be 1.0184x faster <arithmetic> * 6.3775+-0.1268 6.3616+-0.0999 might be 1.0025x faster <geometric> 5.1820+-0.1233 5.1687+-0.1096 might be 1.0026x faster <harmonic> 4.1174+-0.1305 4.1101+-0.1179 might be 1.0018x faster trunk refactor V8: crypto 66.6464+-0.5153 ? 67.0794+-0.8050 ? deltablue 146.7019+-1.9685 146.0889+-0.8053 earley-boyer 81.9158+-1.1611 81.6925+-0.8074 raytrace 47.3773+-0.6577 ? 47.5151+-0.6876 ? regexp 82.2627+-0.4184 ? 82.3784+-0.3202 ? richards 113.0822+-0.9629 112.1683+-0.4324 splay 95.6887+-1.0875 ? 95.9071+-0.5643 ? <arithmetic> 90.5250+-0.5047 90.4042+-0.2401 might be 1.0013x faster <geometric> * 85.6268+-0.4159 85.6057+-0.2542 might be 1.0002x faster <harmonic> 80.8258+-0.3834 ? 80.8900+-0.3075 ? might be 1.0008x slower trunk refactor Kraken: ai-astar 446.0029+-2.1009 445.8995+-1.2161 audio-beat-detection 174.9487+-0.4722 ? 177.2184+-1.9397 ? might be 1.0130x slower audio-dft 344.4122+-3.4931 ? 350.0982+-4.0717 ? might be 1.0165x slower audio-fft 114.6621+-0.7328 ? 115.1699+-0.9732 ? audio-oscillator 231.6501+-4.1325 231.1128+-3.4095 imaging-darkroom 308.3783+-7.1186 ? 308.7481+-6.3187 ? imaging-desaturate 204.0813+-1.4658 203.4175+-0.6241 imaging-gaussian-blur 463.3229+-3.0306 463.0983+-2.4223 json-parse-financial 51.5524+-0.6663 51.1047+-0.1872 json-stringify-tinderbox 64.8833+-0.6971 64.2974+-0.5993 stanford-crypto-aes 92.3050+-0.6536 91.8039+-0.4008 stanford-crypto-ccm 92.4999+-0.8191 ? 92.6817+-0.7803 ? stanford-crypto-pbkdf2 171.7288+-1.2792 ? 171.8636+-1.0281 ? stanford-crypto-sha256-iterative 71.7853+-0.1899 71.7697+-0.5174 <arithmetic> * 202.3009+-1.0974 ? 202.7346+-0.8915 ? might be 1.0021x slower <geometric> 160.1750+-0.7016 ? 160.2683+-0.6107 ? might be 1.0006x slower <harmonic> 126.7766+-0.4536 126.5421+-0.4134 might be 1.0019x faster trunk refactor All benchmarks: <arithmetic> 77.2703+-0.3683 ? 77.3727+-0.2905 ? might be 1.0013x slower <geometric> 21.8632+-0.3058 21.8357+-0.2655 might be 1.0013x faster <harmonic> 7.2173+-0.2221 7.2049+-0.2004 might be 1.0017x faster trunk refactor Geomean of preferred means: <scaled-result> 47.9782+-0.3838 47.9703+-0.2887 might be 1.0002x faster
In summary, performance-neutral, I think. I'm ready for a commit! ;-)
Kind ping to Geoffrey :-)
Unfortunately, I think that my build in Release-trunk was dynamically linking to the build in Release, so my previous results are bogus. For some reason bencher is not running at all for me on trunk, with clean source and build trees, so here are some (pretty poor) sunspider and v8 results for this patch: TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: *1.076x as slow* 157.8ms +/- 0.9% 169.7ms +/- 1.1% significant ============================================================================= 3d: ?? 23.2ms +/- 3.0% 23.9ms +/- 4.8% not conclusive: might be *1.028x as slow* cube: ?? 7.1ms +/- 9.7% 7.5ms +/- 15.1% not conclusive: might be *1.066x as slow* morph: ?? 8.2ms +/- 2.7% 8.5ms +/- 3.7% not conclusive: might be *1.035x as slow* raytrace: - 7.9ms +/- 5.6% 7.8ms +/- 4.7% access: *1.33x as slow* 14.0ms +/- 1.7% 18.7ms +/- 1.2% significant binary-trees: - 2.2ms +/- 7.3% 2.1ms +/- 9.4% fannkuch: *1.81x as slow* 6.1ms +/- 2.3% 11.0ms +/- 1.9% significant nbody: 1.082x as fast 3.2ms +/- 1.8% 2.9ms +/- 4.0% significant nsieve: ?? 2.6ms +/- 7.7% 2.6ms +/- 5.5% not conclusive: might be *1.007x as slow* bitops: *1.28x as slow* 11.8ms +/- 2.1% 15.0ms +/- 1.7% significant 3bit-bits-in-byte: ?? 1.3ms +/- 4.8% 1.4ms +/- 8.1% not conclusive: might be *1.068x as slow* bits-in-byte: ?? 2.3ms +/- 3.6% 2.3ms +/- 4.4% not conclusive: might be *1.023x as slow* bitwise-and: *2.04x as slow* 3.0ms +/- 1.5% 6.1ms +/- 2.0% significant nsieve-bits: - 5.2ms +/- 3.5% 5.2ms +/- 2.9% controlflow: ?? 1.9ms +/- 3.5% 2.0ms +/- 5.0% not conclusive: might be *1.054x as slow* recursive: ?? 1.9ms +/- 3.5% 2.0ms +/- 5.0% not conclusive: might be *1.054x as slow* crypto: 1.160x as fast 12.6ms +/- 3.4% 10.8ms +/- 4.6% significant aes: 1.22x as fast 8.0ms +/- 5.0% 6.5ms +/- 4.4% significant md5: 1.110x as fast 2.5ms +/- 4.5% 2.3ms +/- 9.4% significant sha1: - 2.1ms +/- 2.8% 2.0ms +/- 8.9% date: *1.069x as slow* 19.6ms +/- 1.7% 20.9ms +/- 3.5% significant format-tofte: - 10.5ms +/- 2.3% 10.5ms +/- 2.6% format-xparb: *1.153x as slow* 9.0ms +/- 2.6% 10.4ms +/- 5.9% significant math: *1.28x as slow* 17.3ms +/- 1.4% 22.1ms +/- 0.7% significant cordic: - 6.3ms +/- 1.9% 6.3ms +/- 4.1% partial-sums: *1.57x as slow* 8.6ms +/- 1.8% 13.5ms +/- 1.4% significant spectral-norm: ?? 2.3ms +/- 3.9% 2.3ms +/- 2.9% not conclusive: might be *1.006x as slow* regexp: - 8.4ms +/- 2.6% 8.3ms +/- 2.5% dna: - 8.4ms +/- 2.6% 8.3ms +/- 2.5% string: 1.025x as fast 49.1ms +/- 0.8% 47.9ms +/- 1.4% significant base64: *1.116x as slow* 4.5ms +/- 4.4% 5.0ms +/- 3.5% significant fasta: 1.039x as fast 7.3ms +/- 3.0% 7.0ms +/- 2.2% significant tagcloud: 1.075x as fast 12.0ms +/- 1.4% 11.2ms +/- 2.4% significant unpack-code: 1.048x as fast 19.8ms +/- 1.8% 18.9ms +/- 2.2% significant validate-input: *1.056x as slow* 5.4ms +/- 2.0% 5.8ms +/- 2.5% significant TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: *1.013x as slow* 700.8ms +/- 0.5% 710.0ms +/- 0.8% significant ============================================================================= v8: *1.013x as slow* 700.8ms +/- 0.5% 710.0ms +/- 0.8% significant crypto: 1.016x as fast 72.7ms +/- 0.8% 71.5ms +/- 1.5% significant deltablue: *1.074x as slow* 153.7ms +/- 0.6% 165.0ms +/- 1.3% significant earley-boyer: - 82.8ms +/- 1.9% 82.3ms +/- 1.6% raytrace: - 48.6ms +/- 1.3% 48.2ms +/- 1.2% regexp: ?? 87.4ms +/- 0.8% 87.9ms +/- 1.2% not conclusive: might be *1.007x as slow* richards: - 113.5ms +/- 0.3% 113.1ms +/- 0.8% splay: - 142.1ms +/- 1.0% 141.9ms +/- 0.8%
The slowdown on fannkuch makes me think that somehow we are not emitting the optimal code. I'll look into it.
Created attachment 122912 [details] Patch
Comment on attachment 122912 [details] Patch There were some cases in which resolve_global_dynamic was being emitted instead of resolve_global. With this patch, performance is indeed neutral. Sunspider results: TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: - 155.0ms +/- 0.8% 154.9ms +/- 0.7% ============================================================================= 3d: - 24.5ms +/- 4.0% 23.9ms +/- 3.8% cube: - 8.3ms +/- 11.9% 7.5ms +/- 12.4% morph: 1.044x as fast 8.6ms +/- 3.0% 8.3ms +/- 2.0% significant raytrace: *1.076x as slow* 7.5ms +/- 3.7% 8.1ms +/- 2.7% significant access: ?? 13.4ms +/- 0.9% 13.7ms +/- 2.0% not conclusive: might be *1.019x as slow* binary-trees: *1.040x as slow* 1.9ms +/- 1.0% 2.0ms +/- 3.8% significant fannkuch: ?? 5.8ms +/- 1.8% 5.9ms +/- 2.1% not conclusive: might be *1.008x as slow* nbody: ?? 3.1ms +/- 0.7% 3.2ms +/- 5.3% not conclusive: might be *1.041x as slow* nsieve: ?? 2.6ms +/- 3.7% 2.6ms +/- 1.9% not conclusive: might be *1.005x as slow* bitops: - 11.7ms +/- 1.2% 11.6ms +/- 1.1% 3bit-bits-in-byte: - 1.3ms +/- 5.2% 1.3ms +/- 1.5% bits-in-byte: - 2.2ms +/- 3.0% 2.2ms +/- 1.3% bitwise-and: ?? 3.0ms +/- 0.8% 3.0ms +/- 1.3% not conclusive: might be *1.006x as slow* nsieve-bits: - 5.2ms +/- 1.6% 5.1ms +/- 2.3% controlflow: - 1.9ms +/- 2.3% 1.9ms +/- 0.8% recursive: - 1.9ms +/- 2.3% 1.9ms +/- 0.8% crypto: 1.029x as fast 12.2ms +/- 2.8% 11.9ms +/- 0.5% significant aes: - 7.6ms +/- 3.4% 7.4ms +/- 0.9% md5: - 2.6ms +/- 7.5% 2.4ms +/- 1.6% sha1: - 2.1ms +/- 5.3% 2.1ms +/- 2.2% date: ?? 19.6ms +/- 2.0% 19.9ms +/- 0.7% not conclusive: might be *1.016x as slow* format-tofte: ?? 10.5ms +/- 2.1% 10.5ms +/- 0.7% not conclusive: might be *1.006x as slow* format-xparb: ?? 9.1ms +/- 3.1% 9.4ms +/- 1.7% not conclusive: might be *1.028x as slow* math: - 17.1ms +/- 1.4% 17.0ms +/- 1.2% cordic: ?? 6.2ms +/- 1.7% 6.3ms +/- 1.5% not conclusive: might be *1.012x as slow* partial-sums: - 8.5ms +/- 1.5% 8.5ms +/- 1.3% spectral-norm: - 2.3ms +/- 4.1% 2.3ms +/- 2.1% regexp: ?? 8.2ms +/- 1.7% 8.3ms +/- 2.2% not conclusive: might be *1.008x as slow* dna: ?? 8.2ms +/- 1.7% 8.3ms +/- 2.2% not conclusive: might be *1.008x as slow* string: *1.007x as slow* 46.4ms +/- 0.5% 46.7ms +/- 0.4% significant base64: 1.023x as fast 4.4ms +/- 1.8% 4.3ms +/- 1.2% significant fasta: ?? 6.9ms +/- 1.2% 7.0ms +/- 1.4% not conclusive: might be *1.013x as slow* tagcloud: ?? 11.0ms +/- 1.2% 11.0ms +/- 1.2% not conclusive: might be *1.002x as slow* unpack-code: ?? 18.6ms +/- 0.7% 18.7ms +/- 0.6% not conclusive: might be *1.002x as slow* validate-input: *1.051x as slow* 5.4ms +/- 2.5% 5.7ms +/- 1.3% significant and v8-v6: TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: - 692.3ms +/- 0.7% 692.7ms +/- 0.2% ============================================================================= v8: - 692.3ms +/- 0.7% 692.7ms +/- 0.2% crypto: - 72.2ms +/- 1.1% 72.1ms +/- 0.9% deltablue: ?? 152.0ms +/- 1.2% 153.8ms +/- 0.6% not conclusive: might be *1.011x as slow* earley-boyer: - 80.5ms +/- 1.1% 80.3ms +/- 0.6% raytrace: 1.018x as fast 48.1ms +/- 1.2% 47.2ms +/- 1.0% significant regexp: - 86.5ms +/- 1.1% 86.4ms +/- 0.7% richards: - 112.7ms +/- 1.0% 112.6ms +/- 0.4% splay: - 140.4ms +/- 1.2% 140.3ms +/- 0.3%
Comment on attachment 122912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122912&action=review > Source/JavaScriptCore/ChangeLog:1 > +2012-01-13 Andy Wingo <wingo@igalia.com> Nice ChangeLog -- I like how you commented about individual changes, and the "why" of them. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:507 > +RegisterID* BytecodeGenerator::registerFor(const ResolveResult& location) When you have a trivial declaration like this function argument, I usually prefer the naming style "const ClassName& className". In this case: "const ResolveResult& resolveResult". That way, the one thing doesn't have two names. I think that would be better here. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:512 > + return createLazyRegisterIfNecessary(®isterFor(location.index)); This behavior (creating a lazy register if necessary) should move into BytecodeGenerator::resolve(), since it already has the responsibility of creating the arguments register lazily. It's nice to have all lazy register allocation in one place. (Eventually, we might even give resolve() the responsibility for emitting opcodes like op_resolve and op_get_scoped_var.) > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1137 > + ResolveResult ret = { 0, 0, 0, 0 }; All zeroes should be the default constructor for ResolveResult, so it doesn't have to be specified explicitly. Notice how here you called the ResolveResult "ret", while in other places you called it "location". It would be more uniform to standardize on calling it "resolveResult" everywhere. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1141 > + ret.type = ResolveResult::ImmutableRegister; > + ret.index = m_thisRegister.index(); This data should be private in ResolveResult, with setter functions for the different kinds of resolves. That way, a programmer can't make the error of setting a type that is internally inconsistent with the other information in the ResolveResult. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1153 > + if (entry.isReadOnly()) > + ret.type |= ResolveResult::ImmutableFlag; This would read better if the ResolveResult flag were named "ReadOnlyFlag", to match the "ReadOnly" terminology in SymbolTable. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1429 > +RegisterID* BytecodeGenerator::emitGetStaticVar(RegisterID* dst, const ResolveResult& location) I like how this function turned out. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1438 > + if (dst == ignoredResult()) > + return 0; This would read better, and be slightly more efficient, if you did the early return check before the call to registerFor(). > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:151 > + ResolveResult location = generator.resolve(m_ident); > + if (RegisterID* local = generator.registerFor(location)) { I think you could simplify even further by merging ResolveResult and regsiterFor(), like so: if (RegisterID* local = resolveResult.local()) { // NULL if not a local variable // Code for local variable goes here } In a way, the existing BytecodeGenerator::registerFor() is a weak primordial version of the behavior we want out of BytecodeGenerator::resolve() + ResolveResult.
Comment on attachment 122912 [details] Patch I like how this patch turned out, but I think it will be even better with a round of cleanup before landing.
Thanks for the review; I'll post an updated patch on Monday. Have a lovely weekend.
Created attachment 123574 [details] Patch
(In reply to comment #14) > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:512 > > + return createLazyRegisterIfNecessary(®isterFor(location.index)); > > This behavior (creating a lazy register if necessary) should move into BytecodeGenerator::resolve(), since it already has the responsibility of creating the arguments register lazily. It's nice to have all lazy register allocation in one place. (Eventually, we might even give resolve() the responsibility for emitting opcodes like op_resolve and op_get_scoped_var.) I didn't do this in my new patch, because I misunderstood what you were saying. I'll fix this tomorrow morning if this patch doesn't go in tonight. > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1137 > > + ResolveResult ret = { 0, 0, 0, 0 }; > > All zeroes should be the default constructor for ResolveResult, so it doesn't have to be specified explicitly. I wasn't sure what the right style here was; please have a look. (I'm an old C and Scheme hand, and my C++ could use some improvement.) > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1141 > > + ret.type = ResolveResult::ImmutableRegister; > > + ret.index = m_thisRegister.index(); > > This data should be private in ResolveResult, with setter functions for the different kinds of resolves. That way, a programmer can't make the error of setting a type that is internally inconsistent with the other information in the ResolveResult. It should only be this one function initializing these fields, though I grant that it would be good to make that assumption manifest; a case for a friend class? Or should I add the panoply of getters and setters? (Sorry for the naive questions, I still have bad C++ style instincts :) > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:151 > > + ResolveResult location = generator.resolve(m_ident); > > + if (RegisterID* local = generator.registerFor(location)) { > > I think you could simplify even further by merging ResolveResult and regsiterFor(), like so: > > if (RegisterID* local = resolveResult.local()) { // NULL if not a local variable > // Code for local variable goes here > } > > In a way, the existing BytecodeGenerator::registerFor() is a weak primordial version of the behavior we want out of BytecodeGenerator::resolve() + ResolveResult. You want resolveResult() to hold on to the generator, or have it have a RegisterID* pointer in it? I'm fine with either; the current code reflects my lack of understanding of the memory ownership of registerid values. Thanks for the comments, Andy
> > This behavior (creating a lazy register if necessary) should move into BytecodeGenerator::resolve() > > I didn't do this in my new patch, because I misunderstood what you were saying. I'll fix this tomorrow morning if this patch doesn't go in tonight. Okiedokie. > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1137 > > > + ResolveResult ret = { 0, 0, 0, 0 }; > > > > All zeroes should be the default constructor for ResolveResult, so it doesn't have to be specified explicitly. > > I wasn't sure what the right style here was; please have a look. (I'm an old C and Scheme hand, and my C++ could use some improvement.) + ResolveResult resolveResult = ResolveResult(); You can write this more succinctly as: ResolveResult resolveResult; You need to add a constructor declaration and definition to the ResolveResult class in order for this to do the right thing. Otherwise, you have uninitialized memory. Here's an example definition: inline ResolveResult::ResolveResult() : m_type(0) , m_index(0) , m_depth(0) , m_globalObject(0) { } > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1141 > > > + ret.type = ResolveResult::ImmutableRegister; > > > + ret.index = m_thisRegister.index(); > > > > This data should be private in ResolveResult, with setter functions for the different kinds of resolves. That way, a programmer can't make the error of setting a type that is internally inconsistent with the other information in the ResolveResult. > > It should only be this one function initializing these fields, though I grant that it would be good to make that assumption manifest; a case for a friend class? Or should I add the panoply of getters and setters? (Sorry for the naive questions, I still have bad C++ style instincts :) I would go either with the panoply of (public) getters and setters with (private) data, or with dedicated "create" functions and private data, making ResolveResult a class instead of a struct. These will help make explicit invariants like "If and only if you have a global resolve, you must set type to ResolveResult::Global and globalObject to a JSGlobalObject*". Not better: resolveResult.setType(ResolveResult::Lexical); resolveResult.setDepth(depth); resolveResult.setIndex(entry.getIndex()); return resolveResult; Better: return ResolveResult::lexicalResolve(depth, entry.getIndex()); // Now, the client always knows what data to provide, and doesn't need to know how that data will be encoded. > > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:151 > > > + ResolveResult location = generator.resolve(m_ident); > > > + if (RegisterID* local = generator.registerFor(location)) { > > > > I think you could simplify even further by merging ResolveResult and regsiterFor(), like so: > > > > if (RegisterID* local = resolveResult.local()) { // NULL if not a local variable > > // Code for local variable goes here > > } > > > > In a way, the existing BytecodeGenerator::registerFor() is a weak primordial version of the behavior we want out of BytecodeGenerator::resolve() + ResolveResult. > > You want resolveResult() to hold on to the generator, or have it have a RegisterID* pointer in it? RegisterID*. Temporary RegisterIDs require reference counting. RegisterIDs for local variables are "immortal", and you can refer to them with raw pointers.
Created attachment 123742 [details] Patch
I believe the newly attached patch has addressed all of your comments, Geoffrey. Thanks again for the review.
I don't know how to interpret this, but here's the results of run-webkit-tests --gtk: Unexpected flakiness: text diff mismatch (6) editing/pasteboard/pasting-empty-html-falls-back-to-text.html = TEXT PASS fast/js/navigator-language.html = TEXT PASS http/tests/appcache/abort-cache-ondownloading-resource-404.html = TEXT PASS media/video-poster-blocked-by-willsendrequest.html = TEXT PASS sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.11_Array_prototype_sort/S15.4.4.11_A4_T2.html = TEXT PASS sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.11_Array_prototype_sort/S15.4.4.11_A4_T3.html = TEXT PASS Regressions: Unexpected text diff mismatch : (18) css3/calc/getComputedStyle-margin-percentage.html = TEXT editing/execCommand/insertImage.html = TEXT editing/pasteboard/drag-image-to-contenteditable-in-iframe.html = TEXT fast/block/float/015.html = TEXT fast/borders/rtl-border-05.html = TEXT fast/dom/34176.html = TEXT fast/dom/inner-text.html = TEXT fast/encoding/utf-16-big-endian.html = TEXT fast/encoding/utf-16-little-endian.html = TEXT fast/events/clear-drag-state.html = TEXT fast/events/clear-edit-drag-state.html = TEXT fast/hidpi/broken-image-icon-hidpi.html = TEXT fast/js/array-functions-non-arrays.html = TEXT http/tests/media/video-buffering-repaints-controls.html = TEXT sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.11_Array_prototype_sort/S15.4.4.11_A3_T1.html = TEXT tables/mozilla/bugs/bug2962.html = TEXT tables/mozilla/bugs/bug56201.html = TEXT tables/mozilla/collapsing_borders/bug41262-3.html = TEXT Current as of r105717.
A friendly ping to Geoffrey for review. Cheers, Andy
Geoffrey, would you mind reviewing this patch? Thanks! Andy
Comment on attachment 123742 [details] Patch r=me
Comment on attachment 123742 [details] Patch Clearing flags on attachment: 123742 Committed r106478: <http://trac.webkit.org/changeset/106478>
All reviewed patches have been landed. Closing bug.
This has caused bug 87158.