165031 2016-11-22 09:20:18 -0800 Remove unneeded additional level of short strings optimization in LiteralParser 2017-12-09 15:49:42 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED WONTFIX https://bugs.webkit.org/show_bug.cgi?id=165093 P2 Normal --- 1 darin darin cdumez commit-queue keith_miller mark.lam msaboff rniwa saam sam ysuzuki oldest_to_newest 1253387 0 darin 2016-11-22 09:20:18 -0800 Remove unneeded additional level of optimization in LiteralParser 1253394 1 295338 darin 2016-11-22 09:40:54 -0800 Created attachment 295338 Patch 1253395 2 darin 2016-11-22 09:42:36 -0800 Yusuke, I would like to know your thoughts about this. Am I correct that this is not needed, or did I miss something? 1253399 3 295338 darin 2016-11-22 10:30:21 -0800 Comment on attachment 295338 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295338&action=review > Source/JavaScriptCore/runtime/LiteralParser.cpp:133 > + if (length < 2 || characters[0] >= MaximumCachableCharacter) I think length <= 1 would be easier to understand than length < 2. > Source/JavaScriptCore/runtime/LiteralParser.h:198 > + static constexpr unsigned MaximumCachableCharacter = 128; Just realized this name is not accurate. The maximum "cachable character" is 127, not 128. 1253413 4 ysuzuki 2016-11-22 13:19:34 -0800 (In reply to comment #2) > Yusuke, I would like to know your thoughts about this. Am I correct that > this is not needed, or did I miss something? Agreed. Yeah, we already have the optimization path for empty & 1 length Identifiers in Identifier::add. So, these optimization seems duplicate. BTW, we have similar mechanism for JS parser. You can find it in ParserArena's makeIdentifier. While they need to hold all the identifiers in m_identifiers (to keep its lifetime), caching mechanism by m_recentIdentifiers is the same. Can you drop this thing too? And I think we need to take some performance numbers. Once JS parser change is also done, I recommend to run Octane's CodeLoad. It's parser heavy benchmark. And I also recommend to run kraken and its json benchmark. (But maybe, this benchmark does not contain empty string). 1253414 5 ysuzuki 2016-11-22 13:22:01 -0800 BTW, String::empty is static function, so it will be converted to function calls. If it causes slow down, returning vm.propertyNames->emptyIdentifier in Identifier is one idea. 1253755 6 darin 2016-11-27 10:11:30 -0800 (In reply to comment #4) > BTW, we have similar mechanism for JS parser. You can find it in > ParserArena's makeIdentifier. While they need to hold all the identifiers in > m_identifiers (to keep its lifetime), caching mechanism by > m_recentIdentifiers is the same. > > Can you drop this thing too? I think the situation is different in ParserArena; when we get something out of ParserArena::m_shortIdentifiers, we don't need to add it to ParserArena::m_identifiers again because we already know it’s in there. I suspect that optimization is worthwhile; avoids the possibility of needing to grow m_identifiers for example. But in LiteralParser, there is no such benefit. On the other > And I think we need to take some performance numbers. Once JS parser change > is also done, I recommend to run Octane's CodeLoad. It's parser heavy > benchmark. > And I also recommend to run kraken and its json benchmark. (But maybe, this > benchmark does not contain empty string). Good suggestions. > BTW, String::empty is static function, so it will be converted to function calls. I knew it was a static function, but I am really surprised it’s not inlined. > If it causes slow down, returning vm.propertyNames->emptyIdentifier in Identifier is > one idea. Agreed. 1253766 7 darin 2016-11-27 12:10:24 -0800 Looks like the single character code path in SmallStrings also has a function call. So this patch is not quite right as-is. 1253814 8 ysuzuki 2016-11-27 21:36:21 -0800 (In reply to comment #6) > (In reply to comment #4) > > BTW, we have similar mechanism for JS parser. You can find it in > > ParserArena's makeIdentifier. While they need to hold all the identifiers in > > m_identifiers (to keep its lifetime), caching mechanism by > > m_recentIdentifiers is the same. > > > > Can you drop this thing too? > > I think the situation is different in ParserArena; when we get something out > of ParserArena::m_shortIdentifiers, we don't need to add it to > ParserArena::m_identifiers again because we already know it’s in there. I > suspect that optimization is worthwhile; avoids the possibility of needing > to grow m_identifiers for example. But in LiteralParser, there is no such > benefit. Oh, right. > > On the other > > > And I think we need to take some performance numbers. Once JS parser change > > is also done, I recommend to run Octane's CodeLoad. It's parser heavy > > benchmark. > > And I also recommend to run kraken and its json benchmark. (But maybe, this > > benchmark does not contain empty string). > > Good suggestions. > > > BTW, String::empty is static function, so it will be converted to function calls. > > I knew it was a static function, but I am really surprised it’s not inlined. > I think this should be inlined. The way in my mind is, (1): defining `constexpr` StringImpl constructor (2): defining static StringImpl for empty string (3): returning the address of (2) in StringImpl::empty() I'll attempt to create the patch for this. In particular, (1) is nice. Once it is done, we can easily define static strings in C++. > > If it causes slow down, returning vm.propertyNames->emptyIdentifier in Identifier is > > one idea. > > Agreed. 1253849 9 darin 2016-11-28 09:14:38 -0800 I’ve found lots of non-inlined things that I thought were inlined. There is some work needed to make SmallStrings give inline access to the single character strings. Separately, I am confused by the many different empty strings, and particularly the issues when using empty strings on multiple threads. 1380020 10 darin 2017-12-09 15:49:42 -0800 I’m not going to bother with this. At least not keeping a bug open about it. 295338 2016-11-22 09:40:54 -0800 2016-11-28 09:13:49 -0800 Patch bug-165031-20161122094040.patch text/plain 5951 darin U3VidmVyc2lvbiBSZXZpc2lvbjogMjA4OTYzCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA1 MTY3NjE2MjYyMWE5MzBkNmRjNjNmZjk2YWUyYzUwYWY4MjYxZDRiLi4wYTY3Y2E5NDM3NjQ2ODcw ZTg0ZTI0ODVmYjg0NDI4N2Y1YjIyZWUxIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwzMiBAQAorMjAxNi0xMS0yMiAgRGFyaW4gQWRsZXIgIDxkYXJpbkBhcHBsZS5jb20+CisK KyAgICAgICAgUmVtb3ZlIHVubmVlZGVkIGFkZGl0aW9uYWwgbGV2ZWwgb2Ygb3B0aW1pemF0aW9u IGluIExpdGVyYWxQYXJzZXIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTE2NTAzMQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIEluIFNwZWVkb21ldGVyIHByb2ZpbGVzLCB0aGUgTGl0ZXJhbFBhcnNlciBkZXN0 cnVjdG9yIHdhcyBzaG93aW5nIHVwLAorICAgICAgICBzcGVjaWZpY2FsbHkgdGhlIGRlc3RydWN0 b3JzIGZvciBtX3Nob3J0SWRlbnRpZmllcnMgYW5kIG1fcmVjZW50SWRlbnRpZmllcnMuCisKKyAg ICAgICAgSlNDOjpJZGVudGlmaWVyIGFscmVhZHkgaGFzIGFuIG9wdGltaXplZCBxdWljayBwYXRo IGZvciBlbXB0eSBzdHJpbmdzCisgICAgICAgIGFuZCAxLWNoYXJhY3RlciBzdHJpbmdzLiBUaGVy ZSBpcyBubyBuZWVkIGZvciBMaXRlcmFsUGFyc2VyIHRvIGFkZCBhbgorICAgICAgICBhZGRpdGlv bmFsIG9wdGltaXphdGlvbiBmb3IgdGhvc2UsIGFuZCBzbyBhcyBhIGZpcnN0IHN0ZXAgdG8gYWRk cmVzc2luZworICAgICAgICB0aGUgYWJvdmUgd2UgY2FuIHJlbW92ZSBtX3Nob3J0SWRlbnRpZmll cnMuCisKKyAgICAgICAgQWZ0ZXIgdmVyaWZ5aW5nIHRoYXQgdGhpcyBpcyBlaXRoZXIgbm8gc2xv d2Rvd24sIG9yIGlzIGEgc3BlZWR1cCwgd2Ugc2hvdWxkCisgICAgICAgIG5leHQgY29uc2lkZXIg d2hhdCB0byBkbyBhYm91dCBtX3JlY2VudElkZW50aWZpZXJzOyBwZXJoYXBzIHdlIHNob3VsZCBt b3ZlIHRoYXQKKyAgICAgICAgb3B0aW1pemF0aW9uIHRvIHRoZSBJZGVudGlmaWVyIGxldmVsIGFu ZCBwdXQgdGhlIHJlY2VudCBpZGVudGlmaWVycyBpbnRvIHRoZQorICAgICAgICBWTSByYXRoZXIg dGhhbiBpbiBlYWNoIExpdGVyYWxQYXJzZXIgc28gd2UgZG9uJ3QgaGF2ZSB0byBpdGVyYXRlIG92 ZXIgYWxsIG9mCisgICAgICAgIHRoZW0gZWFjaCB0aW1lIHdlIGRlc3Ryb3kgYSBMaXRlcmFsUGFy c2VyLgorCisgICAgICAgICogcnVudGltZS9MaXRlcmFsUGFyc2VyLmNwcDoKKyAgICAgICAgKEpT Qzo6TGl0ZXJhbFBhcnNlcjxDaGFyYWN0ZXJUeXBlPjo6bWFrZUlkZW50aWZpZXIpOiBSZXBsYWNl ZCB0aGUgdHdvIGZ1bmN0aW9ucworICAgICAgICB3aXRoIGEgc2luZ2xlIGZ1bmN0aW9uIHRlbXBs YXRlIHRoYXQgd29ya3MgZm9yIGJvdGggOC1iaXQgYW5kIDE2LWJpdCBzdHJpbmdzLgorICAgICAg ICBSZW1vdmVkIHRoZSBzcGVjaWFsIGNhc2UgZm9yIGVtcHR5IHN0cmluZyBhbmQgc29tZSBvbmUt Y2hhcmFjdGVyIHN0cmluZ3MuCisKKyAgICAgICAgKiBydW50aW1lL0xpdGVyYWxQYXJzZXIuaDog VXBkYXRlZCBmb3IgdGhlIGFib3ZlIGNoYW5nZSwgYWxzbyByZW1vdmVkCisgICAgICAgIG1fc2hv cnRJZGVudGlmaWVycy4KKwogMjAxNi0xMS0yMSAgTWFyayBMYW0gIDxtYXJrLmxhbUBhcHBsZS5j b20+CiAKICAgICAgICAgUmVtb3ZlZCBhbiBleHRyYSBzcGFjZSBjaGFyYWN0ZXIgYXQgdGhlIGVu ZCBvZiBsaW5lLgpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvTGl0 ZXJhbFBhcnNlci5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9MaXRlcmFsUGFy c2VyLmNwcAppbmRleCA1NmUwMzg5NDdkNjUwNjc0NzE0MzUzYTQ5YzRlOGNiZGQ2OGQ0NDNiLi5k ZjBkNTg5MWZjZGRjODI0Nzk1YjRjY2Y3ODM0YjU2ZjA5NDgyY2ZhIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9MaXRlcmFsUGFyc2VyLmNwcAorKysgYi9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvcnVudGltZS9MaXRlcmFsUGFyc2VyLmNwcApAQCAtMTI4LDQ0ICsxMjgs MTQgQEAgYm9vbCBMaXRlcmFsUGFyc2VyPENoYXJUeXBlPjo6dHJ5SlNPTlBQYXJzZShWZWN0b3I8 SlNPTlBEYXRhPiYgcmVzdWx0cywgYm9vbCBuZWUKICAgICByZXR1cm4gbV9sZXhlci5jdXJyZW50 VG9rZW4oKS0+dHlwZSA9PSBUb2tFbmQ7CiB9CiAgICAgCi10ZW1wbGF0ZSA8dHlwZW5hbWUgQ2hh clR5cGU+Ci1BTFdBWVNfSU5MSU5FIGNvbnN0IElkZW50aWZpZXIgTGl0ZXJhbFBhcnNlcjxDaGFy VHlwZT46Om1ha2VJZGVudGlmaWVyKGNvbnN0IExDaGFyKiBjaGFyYWN0ZXJzLCBzaXplX3QgbGVu Z3RoKQotewotICAgIGlmICghbGVuZ3RoKQotICAgICAgICByZXR1cm4gbV9leGVjLT52bSgpLnBy b3BlcnR5TmFtZXMtPmVtcHR5SWRlbnRpZmllcjsKLSAgICBpZiAoY2hhcmFjdGVyc1swXSA+PSBN YXhpbXVtQ2FjaGFibGVDaGFyYWN0ZXIpCi0gICAgICAgIHJldHVybiBJZGVudGlmaWVyOjpmcm9t U3RyaW5nKCZtX2V4ZWMtPnZtKCksIGNoYXJhY3RlcnMsIGxlbmd0aCk7Ci0KLSAgICBpZiAobGVu Z3RoID09IDEpIHsKLSAgICAgICAgaWYgKCFtX3Nob3J0SWRlbnRpZmllcnNbY2hhcmFjdGVyc1sw XV0uaXNOdWxsKCkpCi0gICAgICAgICAgICByZXR1cm4gbV9zaG9ydElkZW50aWZpZXJzW2NoYXJh Y3RlcnNbMF1dOwotICAgICAgICBtX3Nob3J0SWRlbnRpZmllcnNbY2hhcmFjdGVyc1swXV0gPSBJ ZGVudGlmaWVyOjpmcm9tU3RyaW5nKCZtX2V4ZWMtPnZtKCksIGNoYXJhY3RlcnMsIGxlbmd0aCk7 Ci0gICAgICAgIHJldHVybiBtX3Nob3J0SWRlbnRpZmllcnNbY2hhcmFjdGVyc1swXV07Ci0gICAg fQotICAgIGlmICghbV9yZWNlbnRJZGVudGlmaWVyc1tjaGFyYWN0ZXJzWzBdXS5pc051bGwoKSAm JiBJZGVudGlmaWVyOjplcXVhbChtX3JlY2VudElkZW50aWZpZXJzW2NoYXJhY3RlcnNbMF1dLmlt cGwoKSwgY2hhcmFjdGVycywgbGVuZ3RoKSkKLSAgICAgICAgcmV0dXJuIG1fcmVjZW50SWRlbnRp ZmllcnNbY2hhcmFjdGVyc1swXV07Ci0gICAgbV9yZWNlbnRJZGVudGlmaWVyc1tjaGFyYWN0ZXJz WzBdXSA9IElkZW50aWZpZXI6OmZyb21TdHJpbmcoJm1fZXhlYy0+dm0oKSwgY2hhcmFjdGVycywg bGVuZ3RoKTsKLSAgICByZXR1cm4gbV9yZWNlbnRJZGVudGlmaWVyc1tjaGFyYWN0ZXJzWzBdXTsK LX0KLQotdGVtcGxhdGUgPHR5cGVuYW1lIENoYXJUeXBlPgotQUxXQVlTX0lOTElORSBjb25zdCBJ ZGVudGlmaWVyIExpdGVyYWxQYXJzZXI8Q2hhclR5cGU+OjptYWtlSWRlbnRpZmllcihjb25zdCBV Q2hhciogY2hhcmFjdGVycywgc2l6ZV90IGxlbmd0aCkKK3RlbXBsYXRlPHR5cGVuYW1lIENoYXJh Y3RlclR5cGU+IHRlbXBsYXRlPHR5cGVuYW1lIElkZW50aWZpZXJDaGFyYWN0ZXJUeXBlPiBBTFdB WVNfSU5MSU5FIElkZW50aWZpZXIgTGl0ZXJhbFBhcnNlcjxDaGFyYWN0ZXJUeXBlPjo6bWFrZUlk ZW50aWZpZXIoY29uc3QgSWRlbnRpZmllckNoYXJhY3RlclR5cGUqIGNoYXJhY3RlcnMsIHNpemVf dCBsZW5ndGgpCiB7Ci0gICAgaWYgKCFsZW5ndGgpCi0gICAgICAgIHJldHVybiBtX2V4ZWMtPnZt KCkucHJvcGVydHlOYW1lcy0+ZW1wdHlJZGVudGlmaWVyOwotICAgIGlmIChjaGFyYWN0ZXJzWzBd ID49IE1heGltdW1DYWNoYWJsZUNoYXJhY3RlcikKKyAgICBpZiAobGVuZ3RoIDwgMiB8fCBjaGFy YWN0ZXJzWzBdID49IE1heGltdW1DYWNoYWJsZUNoYXJhY3RlcikKICAgICAgICAgcmV0dXJuIElk ZW50aWZpZXI6OmZyb21TdHJpbmcoJm1fZXhlYy0+dm0oKSwgY2hhcmFjdGVycywgbGVuZ3RoKTsK LQotICAgIGlmIChsZW5ndGggPT0gMSkgewotICAgICAgICBpZiAoIW1fc2hvcnRJZGVudGlmaWVy c1tjaGFyYWN0ZXJzWzBdXS5pc051bGwoKSkKLSAgICAgICAgICAgIHJldHVybiBtX3Nob3J0SWRl bnRpZmllcnNbY2hhcmFjdGVyc1swXV07Ci0gICAgICAgIG1fc2hvcnRJZGVudGlmaWVyc1tjaGFy YWN0ZXJzWzBdXSA9IElkZW50aWZpZXI6OmZyb21TdHJpbmcoJm1fZXhlYy0+dm0oKSwgY2hhcmFj dGVycywgbGVuZ3RoKTsKLSAgICAgICAgcmV0dXJuIG1fc2hvcnRJZGVudGlmaWVyc1tjaGFyYWN0 ZXJzWzBdXTsKLSAgICB9Ci0gICAgaWYgKCFtX3JlY2VudElkZW50aWZpZXJzW2NoYXJhY3RlcnNb MF1dLmlzTnVsbCgpICYmIElkZW50aWZpZXI6OmVxdWFsKG1fcmVjZW50SWRlbnRpZmllcnNbY2hh cmFjdGVyc1swXV0uaW1wbCgpLCBjaGFyYWN0ZXJzLCBsZW5ndGgpKQotICAgICAgICByZXR1cm4g bV9yZWNlbnRJZGVudGlmaWVyc1tjaGFyYWN0ZXJzWzBdXTsKLSAgICBtX3JlY2VudElkZW50aWZp ZXJzW2NoYXJhY3RlcnNbMF1dID0gSWRlbnRpZmllcjo6ZnJvbVN0cmluZygmbV9leGVjLT52bSgp LCBjaGFyYWN0ZXJzLCBsZW5ndGgpOwotICAgIHJldHVybiBtX3JlY2VudElkZW50aWZpZXJzW2No YXJhY3RlcnNbMF1dOworICAgIGF1dG8mIHNsb3QgPSBtX3JlY2VudElkZW50aWZpZXJzW2NoYXJh Y3RlcnNbMF1dOworICAgIGlmICghSWRlbnRpZmllcjo6ZXF1YWwoc2xvdC5pbXBsKCksIGNoYXJh Y3RlcnMsIGxlbmd0aCkpCisgICAgICAgIHNsb3QgPSBJZGVudGlmaWVyOjpmcm9tU3RyaW5nKCZt X2V4ZWMtPnZtKCksIGNoYXJhY3RlcnMsIGxlbmd0aCk7CisgICAgcmV0dXJuIHNsb3Q7CiB9CiAK IHRlbXBsYXRlIDx0eXBlbmFtZSBDaGFyVHlwZT4KZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9ydW50aW1lL0xpdGVyYWxQYXJzZXIuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9y dW50aW1lL0xpdGVyYWxQYXJzZXIuaAppbmRleCA4ZGMxNDJjZDg1NmE2MTgxYWFmNzNmZTcwMjNm ZjQ5YzNiOWM1ODZiLi5mYWVlZjY4MTg0MzJjZWFiZDMyNzY1ZjQ1OWU2NjMwZjE5Mzk3YTUzIDEw MDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9MaXRlcmFsUGFyc2VyLmgK KysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvTGl0ZXJhbFBhcnNlci5oCkBAIC0x OTUsMTEgKzE5NSw5IEBAIHByaXZhdGU6CiAgICAgdHlwZW5hbWUgTGl0ZXJhbFBhcnNlcjxDaGFy VHlwZT46OkxleGVyIG1fbGV4ZXI7CiAgICAgUGFyc2VyTW9kZSBtX21vZGU7CiAgICAgU3RyaW5n IG1fcGFyc2VFcnJvck1lc3NhZ2U7Ci0gICAgc3RhdGljIHVuc2lnbmVkIGNvbnN0IE1heGltdW1D YWNoYWJsZUNoYXJhY3RlciA9IDEyODsKLSAgICBzdGQ6OmFycmF5PElkZW50aWZpZXIsIE1heGlt dW1DYWNoYWJsZUNoYXJhY3Rlcj4gbV9zaG9ydElkZW50aWZpZXJzOworICAgIHN0YXRpYyBjb25z dGV4cHIgdW5zaWduZWQgTWF4aW11bUNhY2hhYmxlQ2hhcmFjdGVyID0gMTI4OwogICAgIHN0ZDo6 YXJyYXk8SWRlbnRpZmllciwgTWF4aW11bUNhY2hhYmxlQ2hhcmFjdGVyPiBtX3JlY2VudElkZW50 aWZpZXJzOwotICAgIEFMV0FZU19JTkxJTkUgY29uc3QgSWRlbnRpZmllciBtYWtlSWRlbnRpZmll cihjb25zdCBMQ2hhciogY2hhcmFjdGVycywgc2l6ZV90IGxlbmd0aCk7Ci0gICAgQUxXQVlTX0lO TElORSBjb25zdCBJZGVudGlmaWVyIG1ha2VJZGVudGlmaWVyKGNvbnN0IFVDaGFyKiBjaGFyYWN0 ZXJzLCBzaXplX3QgbGVuZ3RoKTsKKyAgICB0ZW1wbGF0ZTx0eXBlbmFtZSBJZGVudGlmaWVyQ2hh cmFjdGVyVHlwZT4gQUxXQVlTX0lOTElORSBJZGVudGlmaWVyIG1ha2VJZGVudGlmaWVyKGNvbnN0 IElkZW50aWZpZXJDaGFyYWN0ZXJUeXBlKiBjaGFyYWN0ZXJzLCBzaXplX3QgbGVuZ3RoKTsKIH07 CiAKIH0gLy8gbmFtZXNwYWNlIEpTQwo=