40970 2010-06-21 22:54:51 -0700 JSArrary can use the 'length' property as a storage size hint 2010-06-29 10:22:40 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All UNCONFIRMED P2 Enhancement --- 39115 0 cmarcelo webkit-unassigned ggaren jedrzej.nowacki kent.hansen oldest_to_newest 241126 0 cmarcelo 2010-06-21 22:54:51 -0700 When passing just one argument to the Array constructor in JavaScript, and its argument is an integer number, it'll create an array with the length equal to the argument, and all the values will be empty. This is implemented internally as a JSArray constructor that takes the length and preallocate part of the internal vector in the array. This behaviour is not available for the JSObjectMakeArray in C API, and the alternative of just setting the length property doesn't necessarily will give us the same behaviour as the JavaScript ctor (in this case it seems it doesn't preallocate the internal vector). We could reuse an invalid input of JSObjectMakeArray, which is argumentsCount > 0 but arguments = NULL (a previously invalid, but not checked, input), to get this functionality. It would mean that one wants to construct an Array with length of argumentCount but since no arguments were given, it would be filled with empty values. 241129 1 59341 cmarcelo 2010-06-21 22:59:26 -0700 Created attachment 59341 Patch v1 This is my take on the issue. Of course I would be glad to hear suggestions if you think there's a better way to handle this :) 241870 2 ggaren 2010-06-23 09:03:17 -0700 I see two requests in this bug. 1. "I want a way to create an array with an initial length". I don't think it's a good idea to match the quirk of the JavaScript Array constructor here. It's a very confusing behavior to the uninitiated, and one we intentionally avoided in the API design. The simple way to get this behavior in the current API is to make an array and then set its length property. That seems sufficient to me. Plus, an author who really really wants the quirks of the Array constructor can just call the Array constructor. 2. "I want a way to preallocate the Array's internal storage, for performance reasons". Can you give me an example of a case where the Array's built-in behavior is insufficient? If there is such a case, I think the proper solution is to fix the built-in behavior, instead of adding to the C API. We want all array clients to go fast, not just C API clients. 241872 3 59341 ggaren 2010-06-23 09:03:52 -0700 Comment on attachment 59341 Patch v1 r- because I don't think this change is warranted, but maybe you can convince me. 243777 4 cmarcelo 2010-06-28 11:19:57 -0700 Thanks for the comments, Geoffrey. While I agree this is a quirk in the JavaScript Array API, JSC actually does something different instead of simply creating an empty Array and setting the length. It allocates storage in some situations and fill it with empty values. So, JSC code already optimizes this case for Array constructor (even calling it a quirk). The patch would just allow C API users to access this case just like JS Array constructor provides us. > We want all array clients to go fast, not just C API clients. I might misunderstood your comment, but the patch seems to work in the opposite direction. Enable C API clients to have same access as JavaScript clients have. The context of this patch is supporting the QScriptEngine::newArray(uint length), which follows the quirk on the builtin Array constructor. We can, of course, live with the empty followed by setLength(). But just feels a little bit "odd" since we do have special tuned code for this exact case, i.e. JSC internally optimizes this case instead of simply treating as a quirk. 243867 5 ggaren 2010-06-28 13:31:37 -0700 > We can, of course, live with the empty followed by setLength(). But just feels a little bit "odd" since we do have special tuned code for this exact case, i.e. JSC internally optimizes this case instead of simply treating as a quirk. Don't read too much into the current optimization. It will probably change in the future. Though the current optimization is written for the JSArray constructor, I think a better solution going forward would be for JSArray to take any assignment to .length -- either an implicit assignment by the constructor quirk, or an explicit assignment by API or the .length property -- as a storage size hint. It would also be nice if JSArray could allocate spare storage larger than MIN_SPARSE_ARRAY_INDEX, at least for a few arrays, reclaiming wasted / fragmented space as needed. It's best not to expose these kinds of details in complex and quirky APIs. It might seem like the right thing to do looking at JavaScriptCore from the inside out, but API should be designed for understanding from the outside in. 244037 6 cmarcelo 2010-06-28 18:55:39 -0700 Ok. I agree now that extending C API in this case isn't desirable. > Though the current optimization is written for the JSArray constructor, I think a better solution going forward would be for JSArray to take any assignment to .length -- either an implicit assignment by the constructor quirk, or an explicit assignment by API or the .length property -- as a storage size hint. This seems to be a better solution and keeps the C API clean. > It would also be nice if JSArray could allocate spare storage larger than MIN_SPARSE_ARRAY_INDEX, at least for a few arrays, reclaiming wasted / fragmented space as needed. You mean allowing a larger vector (when e.g. it was explicitly asked) even if it doesn't satisfy the minimum threshold that today's code use? If so, when the reclaim could take place? > It's best not to expose these kinds of details in complex and quirky APIs. It might seem like the right thing to do looking at JavaScriptCore from the inside out, but API should be designed for understanding from the outside in. I'll keep that advice in mind. Thanks. "Operational" question: should this bug be marked as WONTFIX or RESOLVED and another one created for the "use 'length' as storage size hint"? 244153 7 jedrzej.nowacki 2010-06-29 01:20:52 -0700 (In reply to comment #6) > "Operational" question: should this bug be marked as WONTFIX or RESOLVED and another one created for the "use 'length' as storage size hint"? I've changed summary. 244354 8 ggaren 2010-06-29 10:22:40 -0700 > > It would also be nice if JSArray could allocate spare storage larger than MIN_SPARSE_ARRAY_INDEX, at least for a few arrays, reclaiming wasted / fragmented space as needed. > > You mean allowing a larger vector (when e.g. it was explicitly asked) even if it doesn't satisfy the minimum threshold that today's code use? ...even if it exceeds the maximum threshold. > If so, when the reclaim could take place? Probably at GC time. Maybe on a round-robin basis as new Array buffers are allocated. > "Operational" question: should this bug be marked as WONTFIX or RESOLVED and another one created for the "use 'length' as storage size hint"? Probably WONTFIX, with a new bug for using "length" as a storage hint, and another bug for using storage hints more aggressively. 59341 2010-06-21 22:59:26 -0700 2010-06-23 09:03:51 -0700 Patch v1 0001-Add-equivalent-of-one-argument-Array-constructor-fun.patch text/plain 5196 cmarcelo RnJvbSBlZDEzNWMxOTBiNzdkMzIxNDgyZTU5ZmNlYzVlN2QzMTU2YjU0OGY0IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBDYWlvIE1hcmNlbG8gZGUgT2xpdmVpcmEgRmlsaG8gPGNhaW8u b2xpdmVpcmFAb3BlbmJvc3NhLm9yZz4KRGF0ZTogVHVlLCAyMiBKdW4gMjAxMCAwMjozNDozNiAt MDMwMApTdWJqZWN0OiBbUEFUQ0hdIEFkZCBlcXVpdmFsZW50IG9mIG9uZS1hcmd1bWVudCBBcnJh eSBjb25zdHJ1Y3RvciBmdW5jdGlvbmFsaXR5IHRvIEpTQyBDIEFQSQogaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQwOTcwCgpUaGlzIGNvbW1pdCBhZGRzIHRoZSBjYXBh YmlsaXR5IG9mIHRoZSBDIEFQSSB0byBjb25zdHJ1Y3QgYXJyYXlzIHdpdGgKYSBzcGVjaWZpZWQg bGVuZ3RoIGZpbGxlZCB3aXRoIGVtcHR5IHZhbHVlcywgc2ltaWxhciB0byB0aGUgb25lCmFyZ3Vt ZW50IGNvbnN0cnVjdG9yIGluIEpTICJuZXcgQXJyYXkoNDIpIi4KClRvIGFjaGlldmUgdGhhdCB3 aXRob3V0IGFkZGluZyBhIG5ldyBjb25zdHJ1Y3RvciwgYWxsb3cgdGhlCkpTT2JqZWN0TWFrZUFy cmF5KCkgdGFrZSBhIE5VTEwgYXJndW1lbnRzIGV2ZW4gaWYgd2l0aCBhcmd1bWVudENvdW50Cm5v bi16ZXJvLiBUaGlzIHNob3VsZCBub3QgYWZmZWN0IGV4aXN0aW5nIGNvZGUsIHNpbmNlIGl0IHdh cyBpbnZhbGlkCmlucHV0IGJlZm9yZSBhbmQgbm90IGNoZWNrZWQgYnkgdGhlIGZ1bmN0aW9uLgot LS0KIEphdmFTY3JpcHRDb3JlL0FQSS9KU09iamVjdFJlZi5jcHAgfCAgICA2ICsrKystLQogSmF2 YVNjcmlwdENvcmUvQVBJL0pTT2JqZWN0UmVmLmggICB8ICAgIDYgKysrLS0tCiBKYXZhU2NyaXB0 Q29yZS9BUEkvdGVzdHMvdGVzdGFwaS5jIHwgICAgNCArKysrCiBKYXZhU2NyaXB0Q29yZS9DaGFu Z2VMb2cgICAgICAgICAgIHwgICAyMiArKysrKysrKysrKysrKysrKysrKysrCiA0IGZpbGVzIGNo YW5nZWQsIDMzIGluc2VydGlvbnMoKyksIDUgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvSmF2 YVNjcmlwdENvcmUvQVBJL0pTT2JqZWN0UmVmLmNwcCBiL0phdmFTY3JpcHRDb3JlL0FQSS9KU09i amVjdFJlZi5jcHAKaW5kZXggNWUwNTM2Zi4uODMxMDRjMCAxMDA2NDQKLS0tIGEvSmF2YVNjcmlw dENvcmUvQVBJL0pTT2JqZWN0UmVmLmNwcAorKysgYi9KYXZhU2NyaXB0Q29yZS9BUEkvSlNPYmpl Y3RSZWYuY3BwCkBAIC0xNDAsMTMgKzE0MCwxNSBAQCBKU09iamVjdFJlZiBKU09iamVjdE1ha2VB cnJheShKU0NvbnRleHRSZWYgY3R4LCBzaXplX3QgYXJndW1lbnRDb3VudCwgY29uc3QgSlNWYQog ICAgIEFQSUVudHJ5U2hpbSBlbnRyeVNoaW0oZXhlYyk7CiAKICAgICBKU09iamVjdCogcmVzdWx0 OwotICAgIGlmIChhcmd1bWVudENvdW50KSB7CisgICAgaWYgKGFyZ3VtZW50cykgewogICAgICAg ICBNYXJrZWRBcmd1bWVudEJ1ZmZlciBhcmdMaXN0OwogICAgICAgICBmb3IgKHNpemVfdCBpID0g MDsgaSA8IGFyZ3VtZW50Q291bnQ7ICsraSkKICAgICAgICAgICAgIGFyZ0xpc3QuYXBwZW5kKHRv SlMoZXhlYywgYXJndW1lbnRzW2ldKSk7CiAKICAgICAgICAgcmVzdWx0ID0gY29uc3RydWN0QXJy YXkoZXhlYywgYXJnTGlzdCk7Ci0gICAgfSBlbHNlCisgICAgfSBlbHNlIGlmIChhcmd1bWVudENv dW50KQorICAgICAgICByZXN1bHQgPSBjb25zdHJ1Y3RFbXB0eUFycmF5KGV4ZWMsIGFyZ3VtZW50 Q291bnQpOworICAgIGVsc2UKICAgICAgICAgcmVzdWx0ID0gY29uc3RydWN0RW1wdHlBcnJheShl eGVjKTsKIAogICAgIGlmIChleGVjLT5oYWRFeGNlcHRpb24oKSkgewpkaWZmIC0tZ2l0IGEvSmF2 YVNjcmlwdENvcmUvQVBJL0pTT2JqZWN0UmVmLmggYi9KYXZhU2NyaXB0Q29yZS9BUEkvSlNPYmpl Y3RSZWYuaAppbmRleCAzZThiMGViLi5mMzJmMzVjIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9BUEkvSlNPYmplY3RSZWYuaAorKysgYi9KYXZhU2NyaXB0Q29yZS9BUEkvSlNPYmplY3RSZWYu aApAQCAtNDM0LDEyICs0MzQsMTIgQEAgSlNfRVhQT1JUIEpTT2JqZWN0UmVmIEpTT2JqZWN0TWFr ZUNvbnN0cnVjdG9yKEpTQ29udGV4dFJlZiBjdHgsIEpTQ2xhc3NSZWYganNDbGEKICBAZnVuY3Rp b24KICBAYWJzdHJhY3QgQ3JlYXRlcyBhIEphdmFTY3JpcHQgQXJyYXkgb2JqZWN0LgogIEBwYXJh bSBjdHggVGhlIGV4ZWN1dGlvbiBjb250ZXh0IHRvIHVzZS4KLSBAcGFyYW0gYXJndW1lbnRDb3Vu dCBBbiBpbnRlZ2VyIGNvdW50IG9mIHRoZSBudW1iZXIgb2YgYXJndW1lbnRzIGluIGFyZ3VtZW50 cy4KLSBAcGFyYW0gYXJndW1lbnRzIEEgSlNWYWx1ZSBhcnJheSBvZiBkYXRhIHRvIHBvcHVsYXRl IHRoZSBBcnJheSB3aXRoLiBQYXNzIE5VTEwgaWYgYXJndW1lbnRDb3VudCBpcyAwLgorIEBwYXJh bSBhcmd1bWVudENvdW50IEFuIGludGVnZXIgY291bnQgb2YgdGhlIG51bWJlciBvZiBhcmd1bWVu dHMgdG8gcG9wdWxhdGUgdGhlIGFycmF5LgorIEBwYXJhbSBhcmd1bWVudHMgQSBKU1ZhbHVlIGFy cmF5IG9mIGRhdGEgdG8gcG9wdWxhdGUgdGhlIEFycmF5IHdpdGguIFBhc3MgTlVMTCBpZiBhcmd1 bWVudENvdW50IGlzIDAgb3IgaWYgeW91IHdhbnQgdG8gcG9wdWxhdGUgdGhlIGFycmF5IHdpdGgg YXJndW1lbnRDb3VudCBlbXB0eSB2YWx1ZXMuCiAgQHBhcmFtIGV4Y2VwdGlvbiBBIHBvaW50ZXIg dG8gYSBKU1ZhbHVlUmVmIGluIHdoaWNoIHRvIHN0b3JlIGFuIGV4Y2VwdGlvbiwgaWYgYW55LiBQ YXNzIE5VTEwgaWYgeW91IGRvIG5vdCBjYXJlIHRvIHN0b3JlIGFuIGV4Y2VwdGlvbi4KICBAcmVz dWx0IEEgSlNPYmplY3QgdGhhdCBpcyBhbiBBcnJheS4KICBAZGlzY3Vzc2lvbiBUaGUgYmVoYXZp b3Igb2YgdGhpcyBmdW5jdGlvbiBkb2VzIG5vdCBleGFjdGx5IG1hdGNoIHRoZSBiZWhhdmlvciBv ZiB0aGUgYnVpbHQtaW4gQXJyYXkgY29uc3RydWN0b3IuIFNwZWNpZmljYWxseSwgaWYgb25lIGFy Z3VtZW50IAotIGlzIHN1cHBsaWVkLCB0aGlzIGZ1bmN0aW9uIHJldHVybnMgYW4gYXJyYXkgd2l0 aCBvbmUgZWxlbWVudC4KKyBpcyBzdXBwbGllZCwgdGhpcyBmdW5jdGlvbiByZXR1cm5zIGFuIGFy cmF5IHdpdGggb25lIGVsZW1lbnQuIFRvIGdldCB0aGUgYmVoYXZpb3Igb2YgcG9wdWxhdGluZyB3 aXRoIGVtcHR5IHZhbHVlcywgcGFzcyBhIG5vbi16ZXJvIHZhbHVlIHRvIGFyZ3VtZW50Q291bnQg YW5kIE5VTEwgdG8gYXJndW1lbnRzLgogICovCiBKU19FWFBPUlQgSlNPYmplY3RSZWYgSlNPYmpl Y3RNYWtlQXJyYXkoSlNDb250ZXh0UmVmIGN0eCwgc2l6ZV90IGFyZ3VtZW50Q291bnQsIGNvbnN0 IEpTVmFsdWVSZWYgYXJndW1lbnRzW10sIEpTVmFsdWVSZWYqIGV4Y2VwdGlvbikgQVZBSUxBQkxF X0lOX1dFQktJVF9WRVJTSU9OXzRfMDsKIApkaWZmIC0tZ2l0IGEvSmF2YVNjcmlwdENvcmUvQVBJ L3Rlc3RzL3Rlc3RhcGkuYyBiL0phdmFTY3JpcHRDb3JlL0FQSS90ZXN0cy90ZXN0YXBpLmMKaW5k ZXggNzgwZTk5Ni4uMTU2NTc4NSAxMDA2NDQKLS0tIGEvSmF2YVNjcmlwdENvcmUvQVBJL3Rlc3Rz L3Rlc3RhcGkuYworKysgYi9KYXZhU2NyaXB0Q29yZS9BUEkvdGVzdHMvdGVzdGFwaS5jCkBAIC0x Mjc1LDYgKzEyNzUsMTAgQEAgaW50IG1haW4oaW50IGFyZ2MsIGNoYXIqIGFyZ3ZbXSkKICAgICBv ID0gSlNPYmplY3RNYWtlQXJyYXkoY29udGV4dCwgMCwgTlVMTCwgTlVMTCk7CiAgICAgdiA9IEpT T2JqZWN0R2V0UHJvcGVydHkoY29udGV4dCwgbywgc3RyaW5nLCBOVUxMKTsKICAgICBhc3NlcnRF cXVhbHNBc051bWJlcih2LCAwKTsKKworICAgIG8gPSBKU09iamVjdE1ha2VBcnJheShjb250ZXh0 LCA0MiwgTlVMTCwgTlVMTCk7CisgICAgdiA9IEpTT2JqZWN0R2V0UHJvcGVydHkoY29udGV4dCwg bywgc3RyaW5nLCBOVUxMKTsKKyAgICBhc3NlcnRFcXVhbHNBc051bWJlcih2LCA0Mik7CiAgICAg SlNTdHJpbmdSZWxlYXNlKHN0cmluZyk7CiAKICAgICBKU1ZhbHVlUmVmIGFyZ3VtZW50c0RhdGVW YWx1ZXNbXSA9IHsgSlNWYWx1ZU1ha2VOdW1iZXIoY29udGV4dCwgMCkgfTsKZGlmZiAtLWdpdCBh L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRl eCA0YTJiNWIyLi40NzI2NTZmIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cK KysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjUgQEAKKzIwMTAtMDYt MjEgIENhaW8gTWFyY2VsbyBkZSBPbGl2ZWlyYSBGaWxobyAgPGNhaW8ub2xpdmVpcmFAb3BlbmJv c3NhLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBBZGQgZXF1aXZhbGVudCBvZiBvbmUtYXJndW1lbnQgQXJyYXkgY29uc3RydWN0b3IgZnVuY3Rp b25hbGl0eSB0byBKU0MgQyBBUEkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTQwOTcwCisKKyAgICAgICAgVGhpcyBjb21taXQgYWRkcyB0aGUgY2FwYWJp bGl0eSBvZiB0aGUgQyBBUEkgdG8gY29uc3RydWN0IGFycmF5cyB3aXRoCisgICAgICAgIGEgc3Bl Y2lmaWVkIGxlbmd0aCBmaWxsZWQgd2l0aCBlbXB0eSB2YWx1ZXMsIHNpbWlsYXIgdG8gdGhlIG9u ZQorICAgICAgICBhcmd1bWVudCBjb25zdHJ1Y3RvciBpbiBKUyAibmV3IEFycmF5KDQyKSIuCisK KyAgICAgICAgVG8gYWNoaWV2ZSB0aGF0IHdpdGhvdXQgYWRkaW5nIGEgbmV3IGNvbnN0cnVjdG9y LCBhbGxvdyB0aGUKKyAgICAgICAgSlNPYmplY3RNYWtlQXJyYXkoKSB0YWtlIGEgTlVMTCBhcmd1 bWVudHMgZXZlbiBpZiB3aXRoIGFyZ3VtZW50Q291bnQKKyAgICAgICAgbm9uLXplcm8uIFRoaXMg c2hvdWxkIG5vdCBhZmZlY3QgZXhpc3RpbmcgY29kZSwgc2luY2UgaXQgd2FzIGludmFsaWQKKyAg ICAgICAgaW5wdXQgYmVmb3JlIGFuZCBub3QgY2hlY2tlZCBieSB0aGUgZnVuY3Rpb24uCisKKyAg ICAgICAgKiBBUEkvSlNPYmplY3RSZWYuY3BwOgorICAgICAgICAoSlNPYmplY3RNYWtlQXJyYXkp OgorICAgICAgICAqIEFQSS9KU09iamVjdFJlZi5oOgorICAgICAgICAqIEFQSS90ZXN0cy90ZXN0 YXBpLmM6CisgICAgICAgIChtYWluKToKKwogMjAxMC0wNi0yMSAgT2xpdmVyIEh1bnQgIDxvbGl2 ZXJAYXBwbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEdlb2ZmcmV5IEdhcmVuLgotLSAK MS43LjAuMQoK