185337 2018-05-04 18:49:15 -0700 Support instantiating a String with a StaticStringImpl{&, *} 2019-02-03 23:12:19 -0800 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build All All ASSIGNED https://bugs.webkit.org/show_bug.cgi?id=194212 P2 Normal --- 1 dbates dbates benjamin cdumez cmarcelo ews-watchlist ysuzuki oldest_to_newest 1420985 0 dbates 2018-05-04 18:49:15 -0700 We should support instantiating a String with a StaticStringImpl{&, *} so that we can construct constexpr StaticStringImpls and pass them to functions the take a String without needing to cast it to non-const StaticStringImpl beforehand. It is safe for String to take a const StaticStringImpl and internally cast to a non-const StringImpl because StaticStringImpls are guaranteed to be immutable. This immutability is guaranteed both at runtime (by [1]) and by convention of only instantiating a StaticStringImpl in a constexpr expression. [1] <https://trac.webkit.org/browser/trunk/Source/WTF/wtf/text/StringImpl.h?rev=231268#L330> 1420988 1 339622 dbates 2018-05-04 18:53:11 -0700 Created attachment 339622 Patch 1420994 2 339622 ysuzuki 2018-05-04 19:13:01 -0700 Comment on attachment 339622 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339622&action=review > Source/WTF/ChangeLog:11 > + convention of only instantiating a StaticStringImpl in a constexpr expression. Is this condition maintained for the refcount of StaticStringImpl? StaticStringImpl sets s_refCountFlagIsStaticString = 0x1. Since our StringImpl increments/decrements the refcount by 2, setting 0x1 initial ensures that the refcount never becomes zero even if multiple threads ref/deref it. Even though, the refcount itself is mutated. 1420997 3 dbates 2018-05-04 20:02:07 -0700 (In reply to Yusuke Suzuki from comment #2) > Comment on attachment 339622 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339622&action=review > > > Source/WTF/ChangeLog:11 > > + convention of only instantiating a StaticStringImpl in a constexpr expression. > > Is this condition maintained for the refcount of StaticStringImpl? > StaticStringImpl sets s_refCountFlagIsStaticString = 0x1. > Since our StringImpl increments/decrements the refcount by 2, setting 0x1 > initial ensures that the refcount never becomes zero even if multiple > threads ref/deref it. > Even though, the refcount itself is mutated. :( This patch is wrong. 1421001 4 dbates 2018-05-04 20:08:19 -0700 (In reply to Yusuke Suzuki from comment #2) > Comment on attachment 339622 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339622&action=review > > > Source/WTF/ChangeLog:11 > > + convention of only instantiating a StaticStringImpl in a constexpr expression. > > Is this condition maintained for the refcount of StaticStringImpl? > StaticStringImpl sets s_refCountFlagIsStaticString = 0x1. > Since our StringImpl increments/decrements the refcount by 2, setting 0x1 > initial ensures that the refcount never becomes zero even if multiple > threads ref/deref it. How is it thread-safe to increment and decrement the ref count for a StringImpl? I mean, its ref count is an unsigned and we do not use we do not use locks in StringImpl::ref/deref(). In particular, how is StringImpl::deref() thread-safe? 1421003 5 339622 ysuzuki 2018-05-04 20:17:52 -0700 Comment on attachment 339622 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339622&action=review >>>> Source/WTF/ChangeLog:11 >>>> + convention of only instantiating a StaticStringImpl in a constexpr expression. >>> >>> Is this condition maintained for the refcount of StaticStringImpl? StaticStringImpl sets s_refCountFlagIsStaticString = 0x1. >>> Since our StringImpl increments/decrements the refcount by 2, setting 0x1 initial ensures that the refcount never becomes zero even if multiple threads ref/deref it. >>> Even though, the refcount itself is mutated. >> >> :( This patch is wrong. > > How is it thread-safe to increment and decrement the ref count for a StringImpl? I mean, its ref count is an unsigned and we do not use we do not use locks in StringImpl::ref/deref(). In particular, how is StringImpl::deref() thread-safe? Basic StringImpl is not thread-safe. When moving it around threads, we call `isolateCopy()`. The exception is *static* StringImpl (like, StaticStringImpl. The StringImpl with `isStatic()` flag). In this case, the ref count is set to 0x1. Since there is not thread-safety in StringImpl, the effect of StringImpl::ref/deref() would be lost. But it is OK since Static StringImpl is never destroyed. Threads increment/decrement refcount by 2. (ref += 2 / ref -= 2). But refcount's first bit is always 0x1 in static StringImpl. So while the value of refCount() of static StringImpl becomes bogus, the refcount never becomes zero, and static StringImpl is never destroyed. 339622 2018-05-04 18:53:11 -0700 2018-05-04 20:02:16 -0700 Patch Bug185337.patch text/plain 2202 dbates SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAyMzEzOTQpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBACisyMDE4LTA1LTA0ICBEYW5pZWwgQmF0ZXMgIDxkYWJh dGVzQGFwcGxlLmNvbT4KKworICAgICAgICBTdXBwb3J0IGluc3RhbnRpYXRpbmcgYSBTdHJpbmcg d2l0aCBhIFN0YXRpY1N0cmluZ0ltcGx7JiwgKn0KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTE4NTMzNworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIEFkZHMgc3VwcG9ydCBmb3IgaW5zdGFudGlhdGluZyBhIFN0 cmluZyBvYmplY3Qgd2l0aCBhIGNvbnN0IFN0YXRpY1N0cmluZ0ltcGx7JiwgKn0uCisgICAgICAg IEl0IGlzIHNhZmUgdG8gZG8gdGhpcyBiZWNhdXNlIFN0YXRpY1N0cmluZ0ltcGxzIGFyZSBndWFy YW50ZWVkIHRvIGJlIGltbXV0YWJsZS4gVGhpcworICAgICAgICBpbW11dGFiaWxpdHkgaXMgZ3Vh cmFudGVlZCBib3RoIGF0IHJ1bnRpbWUgKHNlZSBbMV0gZm9yIG1vcmUgZGV0YWlscykgYW5kIGJ5 CisgICAgICAgIGNvbnZlbnRpb24gb2Ygb25seSBpbnN0YW50aWF0aW5nIGEgU3RhdGljU3RyaW5n SW1wbCBpbiBhIGNvbnN0ZXhwciBleHByZXNzaW9uLgorCisgICAgICAgIFsxXSA8aHR0cHM6Ly90 cmFjLndlYmtpdC5vcmcvYnJvd3Nlci90cnVuay9Tb3VyY2UvV1RGL3d0Zi90ZXh0L1N0cmluZ0lt cGwuaD9yZXY9MjMxMjY4I0wzMzA+CisKKyAgICAgICAgKiB3dGYvdGV4dC9XVEZTdHJpbmcuaDoK KyAgICAgICAgKFdURjo6U3RyaW5nOjpTdHJpbmcpOgorCiAyMDE4LTA1LTA0ICBUaW0gSG9ydG9u ICA8dGltb3RoeV9ob3J0b25AYXBwbGUuY29tPgogCiAgICAgICAgIFNoaWZ0IHRvIGEgbG93ZXIt bGV2ZWwgZnJhbWV3b3JrIGZvciBzaW1wbGlmeWluZyBVUkxzCkluZGV4OiBTb3VyY2UvV1RGL3d0 Zi90ZXh0L1dURlN0cmluZy5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvd3RmL3RleHQvV1RG U3RyaW5nLmgJKHJldmlzaW9uIDIzMTM5NCkKKysrIFNvdXJjZS9XVEYvd3RmL3RleHQvV1RGU3Ry aW5nLmgJKHdvcmtpbmcgY29weSkKQEAgLTExNSw4ICsxMTUsOCBAQCBwdWJsaWM6CiAgICAgU3Ry aW5nKFJlZjxBdG9taWNTdHJpbmdJbXBsPiYmKTsKICAgICBTdHJpbmcoUmVmUHRyPEF0b21pY1N0 cmluZ0ltcGw+JiYpOwogCi0gICAgU3RyaW5nKFN0YXRpY1N0cmluZ0ltcGwmKTsKLSAgICBTdHJp bmcoU3RhdGljU3RyaW5nSW1wbCopOworICAgIFN0cmluZyhjb25zdCBTdGF0aWNTdHJpbmdJbXBs Jik7CisgICAgU3RyaW5nKGNvbnN0IFN0YXRpY1N0cmluZ0ltcGwqKTsKIAogICAgIC8vIENvbnN0 cnVjdCBhIHN0cmluZyBmcm9tIGEgY29uc3RhbnQgc3RyaW5nIGxpdGVyYWwuCiAgICAgV1RGX0VY UE9SVF9QUklWQVRFIFN0cmluZyhBU0NJSUxpdGVyYWwpOwpAQCAtNDY4LDEzICs0NjgsMTMgQEAg aW5saW5lIFN0cmluZzo6U3RyaW5nKFJlZlB0cjxBdG9taWNTdHJpbgogewogfQogCi1pbmxpbmUg U3RyaW5nOjpTdHJpbmcoU3RhdGljU3RyaW5nSW1wbCYgc3RyaW5nKQotICAgIDogbV9pbXBsKHJl aW50ZXJwcmV0X2Nhc3Q8U3RyaW5nSW1wbCo+KCZzdHJpbmcpKQoraW5saW5lIFN0cmluZzo6U3Ry aW5nKGNvbnN0IFN0YXRpY1N0cmluZ0ltcGwmIHN0cmluZykKKyAgICA6IG1faW1wbChyZWludGVy cHJldF9jYXN0PFN0cmluZ0ltcGwqPihjb25zdF9jYXN0PFN0YXRpY1N0cmluZ0ltcGwqPigmc3Ry aW5nKSkpCiB7CiB9CiAKLWlubGluZSBTdHJpbmc6OlN0cmluZyhTdGF0aWNTdHJpbmdJbXBsKiBz dHJpbmcpCi0gICAgOiBtX2ltcGwocmVpbnRlcnByZXRfY2FzdDxTdHJpbmdJbXBsKj4oc3RyaW5n KSkKK2lubGluZSBTdHJpbmc6OlN0cmluZyhjb25zdCBTdGF0aWNTdHJpbmdJbXBsKiBzdHJpbmcp CisgICAgOiBtX2ltcGwocmVpbnRlcnByZXRfY2FzdDxTdHJpbmdJbXBsKj4oY29uc3RfY2FzdDxT dGF0aWNTdHJpbmdJbXBsKj4oc3RyaW5nKSkpCiB7CiB9CiAK