128050 2014-02-01 14:00:01 -0800 Keep only captured symbols in CodeBlock symbol tables. 2014-02-03 16:08:12 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 kling kling fpizlo ggaren kling oliver oldest_to_newest 974655 0 kling 2014-02-01 14:00:01 -0800 Keep only captured symbols in CodeBlock symbol tables. 974657 1 222890 kling 2014-02-01 14:08:40 -0800 Created attachment 222890 Patch 974661 2 222890 ggaren 2014-02-01 14:41:17 -0800 Comment on attachment 222890 Patch It looks like the code already dropped uncaptured symbols, and you've changed its approach to reuse an existing table and remove items, rather than create a new table and selectively insert items. Have I read that right? What's the motivation for that change? Is there some other object that points to the BytecodeGenrator's SymbolTable? Or do you think removal will be more efficient than creating a copy or something? (If another object also points to the BytecodeGenrator's symbol table, I think a clearer solution might be to separate that object, and force it to clone the SymbolTable as well.) 974663 3 kling 2014-02-01 14:59:42 -0800 (In reply to comment #2) > (From update of attachment 222890 [details]) > It looks like the code already dropped uncaptured symbols, and you've changed its approach to reuse an existing table and remove items, rather than create a new table and selectively insert items. > > Have I read that right? > > What's the motivation for that change? Is there some other object that points to the BytecodeGenrator's SymbolTable? Or do you think removal will be more efficient than creating a copy or something? > > (If another object also points to the BytecodeGenrator's symbol table, I think a clearer solution might be to separate that object, and force it to clone the SymbolTable as well.) The purpose of this change is to minimize the amount of memory used by SymbolTables hanging off of UnlinkedCodeBlocks. We currently keep the full symbol tables around, despite only ever accessing the captured symbols. FWIW the reason we clone the SymbolTable in the first place is to avoid watchpoint reuse. 974844 4 kling 2014-02-02 15:53:51 -0800 How about this: * Rename CodeBlock::symbolTable() into CodeBlock::capturedSymbolTable(). * Fork off a CapturedSymbolTable class from SymbolTable. * CapturedSymbolTable inherits from UnconditionalFinalizer. * WatchpointCleanup goes away (IIUC the JSGlobalObject symbol table doesn't need this mechanism.) * Fork off a CapturedSymbolTableEntry class from SymbolTableEntry. * CapturedSymbolTableEntry always has a watchpoint set. SymbolTableEntry never has a watchpoint set. * Slim/fat logic in both classes disappears. 975954 5 222890 ggaren 2014-02-03 12:03:58 -0800 Comment on attachment 222890 Patch r=me because this patch is better than the status quo. I think it would be even better to make a new SymbolTable, and just copy the captured things into it. (In the common case, nothing will be captured, so that will be faster.) 975955 6 ggaren 2014-02-03 12:05:34 -0800 (In reply to comment #4) > How about this: > > * Rename CodeBlock::symbolTable() into CodeBlock::capturedSymbolTable(). > * Fork off a CapturedSymbolTable class from SymbolTable. > * CapturedSymbolTable inherits from UnconditionalFinalizer. > * WatchpointCleanup goes away (IIUC the JSGlobalObject symbol table doesn't need this mechanism.) > * Fork off a CapturedSymbolTableEntry class from SymbolTableEntry. > * CapturedSymbolTableEntry always has a watchpoint set. SymbolTableEntry never has a watchpoint set. > * Slim/fat logic in both classes disappears. I think this is right, but I think the JSGlobalObject symbol table will still need to be the version of symbol table that has an UnconditionalFinalizer. 975956 7 ggaren 2014-02-03 12:06:24 -0800 I also think it would be nice for the BytecodeGenerator to stop sharing its SymbolTable* with some other object. Instead, BytecodeGenerator should create its own symbol table, and perform an explicit copy if any other object needs that data. 976079 8 kling 2014-02-03 16:08:12 -0800 Committed r163337: <http://trac.webkit.org/changeset/163337> 222890 2014-02-01 14:08:40 -0800 2014-02-03 12:03:58 -0800 Patch bug-128050.diff text/plain 4853 kling ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDRlMmM4ZDkuLjJmNWQ1OWEgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI0IEBACisyMDE0LTAyLTAxICBBbmRyZWFzIEtsaW5n ICA8YWtsaW5nQGFwcGxlLmNvbT4KKworICAgICAgICBLZWVwIG9ubHkgY2FwdHVyZWQgc3ltYm9s cyBpbiBDb2RlQmxvY2sgc3ltYm9sIHRhYmxlcy4KKyAgICAgICAgPGh0dHBzOi8vd2Via2l0Lm9y Zy9iLzEyODA1MD4KKworICAgICAgICBEaXNjYXJkIGFsbCB1bmNhcHR1cmVkIHN5bWJvbHMgYXQg dGhlIGVuZCBvZiBjb2RlZ2VuIHNpbmNlIG9ubHkKKyAgICAgICAgdGhlIGNhcHR1cmVkIG9uZXMg d2lsbCBiZSB1c2VkIGFmdGVyIHRoYXQgcG9pbnQuCisKKyAgICAgICAgfjJNQiBwcm9ncmVzc2lv biBvbiBNZW1idXN0ZXIgT1NVUy4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICAqIGJ5dGVjb2RlL0NvZGVCbG9jay5jcHA6CisgICAgICAgIChKU0M6OkNv ZGVCbG9jazo6Q29kZUJsb2NrKToKKyAgICAgICAgKiBieXRlY29tcGlsZXIvQnl0ZWNvZGVHZW5l cmF0b3IuY3BwOgorICAgICAgICAoSlNDOjpCeXRlY29kZUdlbmVyYXRvcjo6Z2VuZXJhdGUpOgor ICAgICAgICAqIHJ1bnRpbWUvU3ltYm9sVGFibGUuY3BwOgorICAgICAgICAoSlNDOjpTeW1ib2xU YWJsZTo6cmVtb3ZlVW5jYXB0dXJlZFN5bWJvbHMpOgorICAgICAgICAoSlNDOjpTeW1ib2xUYWJs ZTo6Y2xvbmUpOgorICAgICAgICAqIHJ1bnRpbWUvU3ltYm9sVGFibGUuaDoKKwogMjAxNC0wMS0z MSAgTWljaGFlbCBTYWJvZmYgIDxtc2Fib2ZmQGFwcGxlLmNvbT4KIAogICAgICAgICBSRUdSRVNT SU9OOiBDcmFzaCBpbiBzYW5pdGl6ZVN0YWNrRm9yVk1JbXBsIHdoZW4gc2Nyb2xsaW5nIEAgbGlm ZWhhY2tlci5jb20uYXUKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29k ZS9Db2RlQmxvY2suY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2J5dGVjb2RlL0NvZGVCbG9j ay5jcHAKaW5kZXggODYwMGYxOS4uNzUxNTA2NSAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3Jp cHRDb3JlL2J5dGVjb2RlL0NvZGVCbG9jay5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3Jl L2J5dGVjb2RlL0NvZGVCbG9jay5jcHAKQEAgLTE1NjAsNyArMTU2MCw3IEBAIENvZGVCbG9jazo6 Q29kZUJsb2NrKFNjcmlwdEV4ZWN1dGFibGUqIG93bmVyRXhlY3V0YWJsZSwgVW5saW5rZWRDb2Rl QmxvY2sqIHVubGluCiAgICAgCiAgICAgaWYgKFN5bWJvbFRhYmxlKiBzeW1ib2xUYWJsZSA9IHVu bGlua2VkQ29kZUJsb2NrLT5zeW1ib2xUYWJsZSgpKSB7CiAgICAgICAgIGlmIChjb2RlVHlwZSgp ID09IEZ1bmN0aW9uQ29kZSAmJiBzeW1ib2xUYWJsZS0+Y2FwdHVyZUNvdW50KCkpIHsKLSAgICAg ICAgICAgIG1fc3ltYm9sVGFibGUuc2V0KCptX3ZtLCBtX293bmVyRXhlY3V0YWJsZS5nZXQoKSwg c3ltYm9sVGFibGUtPmNsb25lQ2FwdHVyZWROYW1lcygqbV92bSkpOworICAgICAgICAgICAgbV9z eW1ib2xUYWJsZS5zZXQoKm1fdm0sIG1fb3duZXJFeGVjdXRhYmxlLmdldCgpLCBzeW1ib2xUYWJs ZS0+Y2xvbmUoKm1fdm0pKTsKICAgICAgICAgICAgIGRpZENsb25lU3ltYm9sVGFibGUgPSB0cnVl OwogICAgICAgICB9IGVsc2UKICAgICAgICAgICAgIG1fc3ltYm9sVGFibGUuc2V0KCptX3ZtLCBt X293bmVyRXhlY3V0YWJsZS5nZXQoKSwgc3ltYm9sVGFibGUpOwpkaWZmIC0tZ2l0IGEvU291cmNl L0phdmFTY3JpcHRDb3JlL2J5dGVjb21waWxlci9CeXRlY29kZUdlbmVyYXRvci5jcHAgYi9Tb3Vy Y2UvSmF2YVNjcmlwdENvcmUvYnl0ZWNvbXBpbGVyL0J5dGVjb2RlR2VuZXJhdG9yLmNwcAppbmRl eCBlZjdiOGUwLi40NDZkMzAzIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYnl0 ZWNvbXBpbGVyL0J5dGVjb2RlR2VuZXJhdG9yLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvYnl0ZWNvbXBpbGVyL0J5dGVjb2RlR2VuZXJhdG9yLmNwcApAQCAtMTExLDYgKzExMSw5IEBA IFBhcnNlckVycm9yIEJ5dGVjb2RlR2VuZXJhdG9yOjpnZW5lcmF0ZSgpCiAKICAgICBtX2NvZGVC bG9jay0+c2hyaW5rVG9GaXQoKTsKIAorICAgIGlmIChtX2NvZGVCbG9jay0+c3ltYm9sVGFibGUo KSkKKyAgICAgICAgbV9jb2RlQmxvY2stPnN5bWJvbFRhYmxlKCktPnJlbW92ZVVuY2FwdHVyZWRT eW1ib2xzKCk7CisKICAgICBpZiAobV9leHByZXNzaW9uVG9vRGVlcCkKICAgICAgICAgcmV0dXJu IFBhcnNlckVycm9yKFBhcnNlckVycm9yOjpPdXRPZk1lbW9yeSk7CiAgICAgcmV0dXJuIFBhcnNl ckVycm9yKFBhcnNlckVycm9yOjpFcnJvck5vbmUpOwpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFT Y3JpcHRDb3JlL3J1bnRpbWUvU3ltYm9sVGFibGUuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3Jl L3J1bnRpbWUvU3ltYm9sVGFibGUuY3BwCmluZGV4IDhhZDU1YjEuLjU2MjgzMWIgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1N5bWJvbFRhYmxlLmNwcAorKysgYi9T b3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9TeW1ib2xUYWJsZS5jcHAKQEAgLTEzNCw3ICsx MzQsMjcgQEAgdm9pZCBTeW1ib2xUYWJsZTo6V2F0Y2hwb2ludENsZWFudXA6OmZpbmFsaXplVW5j b25kaXRpb25hbGx5KCkKICAgICB9CiB9CiAKLVN5bWJvbFRhYmxlKiBTeW1ib2xUYWJsZTo6Y2xv bmVDYXB0dXJlZE5hbWVzKFZNJiB2bSkKK3ZvaWQgU3ltYm9sVGFibGU6OnJlbW92ZVVuY2FwdHVy ZWRTeW1ib2xzKCkKK3sKKyAgICBWZWN0b3I8U3RyaW5nSW1wbCo+IGVudHJpZXNUb1JlbW92ZTsK KyAgICBlbnRyaWVzVG9SZW1vdmUucmVzZXJ2ZUluaXRpYWxDYXBhY2l0eShtX21hcC5zaXplKCkp OworCisgICAgZm9yIChhdXRvIGl0IDogbV9tYXApIHsKKyAgICAgICAgaWYgKGlzQ2FwdHVyZWQo aXQudmFsdWUuZ2V0SW5kZXgoKSkpCisgICAgICAgICAgICBjb250aW51ZTsKKyAgICAgICAgZW50 cmllc1RvUmVtb3ZlLnVuY2hlY2tlZEFwcGVuZChpdC5rZXkuZ2V0KCkpOworICAgIH0KKworICAg IGlmIChzdGF0aWNfY2FzdDxpbnQ+KGVudHJpZXNUb1JlbW92ZS5zaXplKCkpID09IG1fbWFwLnNp emUoKSkgeworICAgICAgICBtX21hcC5jbGVhcigpOworICAgICAgICByZXR1cm47CisgICAgfQor CisgICAgZm9yIChhdXRvIGVudHJ5IDogZW50cmllc1RvUmVtb3ZlKQorICAgICAgICBtX21hcC5y ZW1vdmUoZW50cnkpOworfQorCitTeW1ib2xUYWJsZSogU3ltYm9sVGFibGU6OmNsb25lKFZNJiB2 bSkKIHsKICAgICBTeW1ib2xUYWJsZSogcmVzdWx0ID0gU3ltYm9sVGFibGU6OmNyZWF0ZSh2bSk7 CiAgICAgCkBAIC0xNDQsOCArMTY0LDYgQEAgU3ltYm9sVGFibGUqIFN5bWJvbFRhYmxlOjpjbG9u ZUNhcHR1cmVkTmFtZXMoVk0mIHZtKQogICAgIHJlc3VsdC0+bV9jYXB0dXJlRW5kID0gbV9jYXB0 dXJlRW5kOwogCiAgICAgZm9yIChhdXRvIGl0ZXIgPSBtX21hcC5iZWdpbigpLCBlbmQgPSBtX21h cC5lbmQoKTsgaXRlciAhPSBlbmQ7ICsraXRlcikgewotICAgICAgICBpZiAoIWlzQ2FwdHVyZWQo aXRlci0+dmFsdWUuZ2V0SW5kZXgoKSkpCi0gICAgICAgICAgICBjb250aW51ZTsKICAgICAgICAg cmVzdWx0LT5tX21hcC5hZGQoCiAgICAgICAgICAgICBpdGVyLT5rZXksCiAgICAgICAgICAgICBT eW1ib2xUYWJsZUVudHJ5KGl0ZXItPnZhbHVlLmdldEluZGV4KCksIGl0ZXItPnZhbHVlLmdldEF0 dHJpYnV0ZXMoKSkpOwpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUv U3ltYm9sVGFibGUuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1N5bWJvbFRhYmxl LmgKaW5kZXggMDQzNjczNC4uOGU5NjM3NyAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRD b3JlL3J1bnRpbWUvU3ltYm9sVGFibGUuaAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVu dGltZS9TeW1ib2xUYWJsZS5oCkBAIC0zNDYsNiArMzQ2LDggQEAgcHVibGljOgogICAgIHN0YXRp YyBjb25zdCBib29sIGhhc0ltbW9ydGFsU3RydWN0dXJlID0gdHJ1ZTsKICAgICBzdGF0aWMgdm9p ZCBkZXN0cm95KEpTQ2VsbCopOwogCisgICAgdm9pZCByZW1vdmVVbmNhcHR1cmVkU3ltYm9scygp OworCiAgICAgc3RhdGljIFN0cnVjdHVyZSogY3JlYXRlU3RydWN0dXJlKFZNJiB2bSwgSlNHbG9i YWxPYmplY3QqIGdsb2JhbE9iamVjdCwgSlNWYWx1ZSBwcm90b3R5cGUpCiAgICAgewogICAgICAg ICByZXR1cm4gU3RydWN0dXJlOjpjcmVhdGUodm0sIGdsb2JhbE9iamVjdCwgcHJvdG90eXBlLCBU eXBlSW5mbyhMZWFmVHlwZSwgU3RydWN0dXJlRmxhZ3MpLCBpbmZvKCkpOwpAQCAtNDY3LDcgKzQ2 OSw3IEBAIHB1YmxpYzoKICAgICBjb25zdCBTbG93QXJndW1lbnQqIHNsb3dBcmd1bWVudHMoKSB7 IHJldHVybiBtX3Nsb3dBcmd1bWVudHMuZ2V0KCk7IH0KICAgICB2b2lkIHNldFNsb3dBcmd1bWVu dHMoc3RkOjp1bmlxdWVfcHRyPFNsb3dBcmd1bWVudFtdPiBzbG93QXJndW1lbnRzKSB7IG1fc2xv d0FyZ3VtZW50cyA9IHN0ZDo6bW92ZShzbG93QXJndW1lbnRzKTsgfQogICAgIAotICAgIFN5bWJv bFRhYmxlKiBjbG9uZUNhcHR1cmVkTmFtZXMoVk0mKTsKKyAgICBTeW1ib2xUYWJsZSogY2xvbmUo Vk0mKTsKIAogICAgIHN0YXRpYyB2b2lkIHZpc2l0Q2hpbGRyZW4oSlNDZWxsKiwgU2xvdFZpc2l0 b3ImKTsKIAo=