For executable code, JSC::Executable uses a weak handle finalizer. This ensures the executable code is released as soon as the GC knows that the Executable object is no longer reachable. For everything else, JSC::Executable uses a destructor, which may only run when the cell in which JSC::Executable was allocated gets used for a new allocation. This is not a bad approach from a performance standpoint, since releasing executable code as soon as possible is always a good policy. But, the CodeBlock object is itself sizable, so releasing it sooner would be nice for memory usage. It's also true that moving all memory release into a finalizer is a good step towards ultimately not executing destructors. Doing so also makes confusing code slightly less confusing. The code is necessarily confusing because certain state in CodeBlock is stored in Executable to make it quicker to query during function dispatch. The current approach means that the part of CodeBlock's state that is stored in Executable is freed early, so when the CodeBlock is freed later from the destructor, some of its state is already released. It's probably wise to continue to store (or at least cache) some of CodeBlock's state in Executable, but it would make things somewhat easier to understand if the release of CodeBlock memory happened at the same time as the release of CodeBlock state that was cached in Executable.
Created attachment 105424 [details] the patch Tests seem to pass, it seems that with this patch I can still browse the web, and performance appears neutral. Full performance report below. Benchmark report for SunSpider, V8, and Kraken. VMs tested: "TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc "ExecFinalize" at /Volumes/Data/pizlo/quartary/OpenSource/WebKitBuild/Release/jsc Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. 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. TipOfTree ExecFinalize SunSpider: 3d-cube 7.6708+-0.1176 7.5841+-0.1639 might be 1.0114x faster 3d-morph 7.5792+-0.1602 7.3793+-0.1658 might be 1.0271x faster 3d-raytrace 7.6858+-0.1833 7.6331+-0.1749 access-binary-trees 2.3464+-0.1027 2.2665+-0.0545 might be 1.0353x faster access-fannkuch 11.6685+-0.2678 ? 11.7823+-0.2536 ? access-nbody 4.2522+-0.0905 4.1475+-0.0589 might be 1.0252x faster access-nsieve 2.5671+-0.0849 2.4992+-0.0802 might be 1.0272x faster bitops-3bit-bits-in-byte 1.7282+-0.0525 ? 1.7325+-0.0437 ? bitops-bits-in-byte 4.4494+-0.2025 4.4308+-0.1947 bitops-bitwise-and 3.6244+-0.0621 ? 3.6801+-0.0639 ? might be 1.0154x slower bitops-nsieve-bits 5.4832+-0.1309 5.3283+-0.1084 might be 1.0291x faster controlflow-recursive 2.0154+-0.0503 ? 2.1204+-0.0739 ? might be 1.0521x slower crypto-aes 6.4391+-0.2795 ? 6.6213+-0.1974 ? might be 1.0283x slower crypto-md5 2.8146+-0.0728 2.7349+-0.0893 might be 1.0291x faster crypto-sha1 2.2402+-0.0931 2.2180+-0.0429 might be 1.0100x faster date-format-tofte 10.1556+-0.1648 10.0386+-0.1697 might be 1.0117x faster date-format-xparb 8.6652+-0.2455 8.3647+-0.2152 might be 1.0359x faster math-cordic 6.2553+-0.1021 ? 6.4305+-0.1790 ? might be 1.0280x slower math-partial-sums 7.6624+-0.1552 7.5720+-0.1294 might be 1.0119x faster math-spectral-norm 2.4831+-0.0550 2.4610+-0.0428 regexp-dna 10.2313+-0.1627 ? 10.3778+-0.2369 ? might be 1.0143x slower string-base64 6.0103+-0.1399 ? 6.0212+-0.1152 ? string-fasta 7.3684+-0.1139 7.3548+-0.1170 string-tagcloud 13.3408+-0.2701 ? 13.3510+-0.2191 ? string-unpack-code 18.5239+-0.3307 18.4312+-0.2688 string-validate-input 7.3392+-0.3738 7.2632+-0.1464 might be 1.0105x faster <arithmetic> 6.5615+-0.0347 6.5317+-0.0292 <geometric> 5.4509+-0.0349 5.4234+-0.0285 <harmonic> 4.4624+-0.0390 4.4418+-0.0268 TipOfTree ExecFinalize V8: crypto 89.9669+-0.9327 89.8237+-0.5736 deltablue 265.7069+-2.3741 263.3825+-1.1593 earley-boyer 101.6921+-0.7357 101.3654+-0.7025 raytrace 79.4849+-2.3809 77.1534+-0.4093 might be 1.0302x faster regexp 109.1505+-0.7869 ? 110.0358+-1.0691 ? richards 240.8500+-1.5259 ? 242.0161+-1.1816 ? splay 108.6089+-0.7801 ? 108.7020+-1.0020 ? <arithmetic> 142.2086+-0.6081 141.7827+-0.3095 <geometric> 127.6110+-0.7872 127.0863+-0.3010 <harmonic> 116.9864+-0.9733 116.3146+-0.3005 TipOfTree ExecFinalize Kraken: ai-astar 1089.1605+-11.5608 ? 1093.1558+-10.3830 ? audio-beat-detection 469.1552+-2.4998 ? 472.8775+-4.3576 ? audio-dft 423.7785+-12.0481 ? 427.6782+-12.8277 ? audio-fft 366.1977+-0.7915 ! 369.2613+-0.7147 ! definitely 1.0084x slower audio-oscillator 399.8224+-4.1327 397.2567+-2.8732 imaging-darkroom 526.6979+-3.3260 ? 530.2128+-3.4240 ? imaging-desaturate 586.1033+-6.2498 ? 588.2981+-7.4616 ? imaging-gaussian-blur 1704.6206+-4.4376 ? 1705.4177+-5.6552 ? json-parse-financial 48.0213+-0.2319 ! 48.6721+-0.3170 ! definitely 1.0136x slower json-stringify-tinderbox 62.1680+-0.8153 ? 62.6272+-0.6596 ? stanford-crypto-aes 143.6970+-1.8040 ? 144.7303+-1.9498 ? stanford-crypto-ccm 111.4988+-1.1214 111.4743+-0.7385 stanford-crypto-pbkdf2 335.6885+-2.1689 ? 336.1219+-1.8932 ? stanford-crypto-sha256-iterative 129.5380+-0.3716 ? 132.1052+-3.1976 ? might be 1.0198x slower <arithmetic> 456.8677+-1.9499 ? 458.5635+-1.2238 ? <geometric> 293.1670+-1.1406 ? 294.8764+-0.9579 ? <harmonic> 178.6056+-0.5588 ! 180.1183+-0.7114 ! definitely 1.0085x slower TipOfTree ExecFinalize All benchmarks: <arithmetic> 160.8980+-0.6071 ? 161.3232+-0.3711 ? <geometric> 28.5710+-0.1106 28.5233+-0.0851 <harmonic> 7.8796+-0.0674 7.8446+-0.0462
Comment on attachment 105424 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=105424&action=review > Source/JavaScriptCore/runtime/Executable.cpp:251 > + if (m_evalCodeBlock) > + m_evalCodeBlock->clearEvalCache(); > + m_evalCodeBlock.clear(); Could put the clear inside the if. > Source/JavaScriptCore/runtime/Executable.cpp:343 > + if (m_programCodeBlock) > + m_programCodeBlock->clearEvalCache(); > + m_programCodeBlock.clear(); Could put the clear inside the if. > Source/JavaScriptCore/runtime/Executable.cpp:496 > if (m_codeBlockForCall) > m_codeBlockForCall->clearEvalCache(); > m_codeBlockForCall.clear(); Could put the clear inside the if. > Source/JavaScriptCore/runtime/Executable.cpp:499 > if (m_codeBlockForConstruct) > m_codeBlockForConstruct->clearEvalCache(); > m_codeBlockForConstruct.clear(); Could put the clear inside the if. > Source/JavaScriptCore/runtime/Executable.h:-69 > -#if ENABLE(JIT) > Weak<ExecutableBase> finalizer(globalData, this, executableFinalizer()); > finalizer.leakHandle(); > -#endif Would be nice to have a comment in the change log explaining this change. > Source/JavaScriptCore/runtime/Executable.h:337 > + virtual void clearCode(); I like to follow a pattern where virtual functions overrides are private or protected, even when the base class function is public. This sometimes lets us discover cases where we would be doing a virtual function dispatch and could instead do a non-virtual, and has little cost. Accordingly, I suggest making this protected and private in derived classes, and leave it public only in ExecutableBase.
Created attachment 105442 [details] the patch (fix review)
Comment on attachment 105442 [details] the patch (fix review) Clearing flags on attachment: 105442 Committed r93947: <http://trac.webkit.org/changeset/93947>
All reviewed patches have been landed. Closing bug.