125172 2013-12-03 11:34:21 -0800 ObjectAllocationProfile is racy and the DFG should be cool with that 2013-12-06 13:27:19 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 1 fpizlo fpizlo barraclough darin ggaren mark.lam mhahnenberg msaboff oliver sam oldest_to_newest 956007 0 fpizlo 2013-12-03 11:34:21 -0800 Patch forthcoming <rdar://problem/15233487> 956014 1 218318 fpizlo 2013-12-03 11:36:08 -0800 Created attachment 218318 the patch 956015 2 218318 mhahnenberg 2013-12-03 11:38:30 -0800 Comment on attachment 218318 the patch r=me 956100 3 fpizlo 2013-12-03 14:22:18 -0800 Landed in http://trac.webkit.org/changeset/160038 956112 4 218318 darin 2013-12-03 14:31:00 -0800 Comment on attachment 218318 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=218318&action=review > Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:156 > + ASSERT(structure); When you find yourself writing this assertion, it’s worth considering making the argument Structure& instead of Structure*. > Source/JavaScriptCore/runtime/JSFunction.h:141 > + Structure* allocationStructure() { return m_allocationProfile.structure(); } If this can never be null we should consider returning Structure&. 956128 5 fpizlo 2013-12-03 14:41:30 -0800 (In reply to comment #4) > (From update of attachment 218318 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=218318&action=review > > > Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:156 > > + ASSERT(structure); > > When you find yourself writing this assertion, it’s worth considering making the argument Structure& instead of Structure*. This would be strange. We use * for JSC heap pointers and Structure is a JSC heap object. There is no precedent for using & to refer to JSC heap objects. I expect accesses to the JSC heap to use -> and not ".". It would be weird to me if the same kind of object was referred to using both & and *. This is confusing; you end up with a mix of . and -> to access its members, and you find yourself having to use & and * in a bunch of places to convert from one pointer style to the other. We have this issue with JSC::VM and it's really gross. But overall, I just really don't like &. I wish it wasn't part of the language. > > > Source/JavaScriptCore/runtime/JSFunction.h:141 > > + Structure* allocationStructure() { return m_allocationProfile.structure(); } > > If this can never be null we should consider returning Structure&. It can be null, and that's OK. 956943 6 darin 2013-12-05 10:03:32 -0800 (In reply to comment #5) > But overall, I just really don't like &. I wish it wasn't part of the language. OK, that seems like the real issue. We need to get you together with the folks currently setting the style elsewhere in WebKit, because elsewhere we are seeing improvements that come from switching from pointers to references. 957372 7 fpizlo 2013-12-06 13:27:19 -0800 (In reply to comment #6) > (In reply to comment #5) > > But overall, I just really don't like &. I wish it wasn't part of the language. > > OK, that seems like the real issue. > > We need to get you together with the folks currently setting the style elsewhere in WebKit, because elsewhere we are seeing improvements that come from switching from pointers to references. I think that the bigger issue for this particular case is just that we always use pointers, not references, to refer to things allocated in the JS heap (i.e. JSCell and friends). We then leverage this with idioms that either allow or require JSCell* to possibly be null, for example: if (JSObject* object = jsDynamicCast<JSObject*>(jsValue)) { // do things for the case that jsValue is an object. } else { // do things for the case that it ain't } This is sort of fitting because of the dynamic nature of JSCells and JSValues. They are the C++-side proxies for values in a dynamic language, and so it is quite rare that we have a guarantee like "I know that I have a pointer to Blah and it's not null". There may be a separate debate regarding whether & is better than * for WebCore or broadly for "most objects" in WebKit. Even if the outcome of that debate is (or was, past tense) that & is usually better, I still think it's sensible to ask questions like: - Does it make the code better, or worse, if we make one particular reference to an object use & even though it's a convention to always use * for that object? - Is it a good use of time to attempt to change all or some of the * pointers to a kind of object into & references? - If we incrementally start changing all references to an object to be & instead of *, how much of a cost will this incur in the meantime for other people trying to do things to the codebase? For JSCell, I propose that the answers are, respectively: worse, bad use, too much. The reason for my pessimism is that I would guess that there will be very few places where we can reasonably use & instead of * for JSCells and so those few places will just stick out like sore thumbs. For example, they will degrade readability because those of us who stare at the code the most are accustomed to "." and "&" meaning "it ain't GC'd". Getting to the point where we find those places (where JSCell& would work at all) will take a non-trivial amount of effort and will produce relatively small gains. The trade-off may be different for other kinds of objects. Personally, I would always err on the side of * instead of &, but in my arguments above I'm trying to step back from that bias and make a more objective argument. 218318 2013-12-03 11:36:08 -0800 2013-12-03 14:31:00 -0800 the patch blah.patch text/plain 5000 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTYwMDIxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIzIEBA CisyMDEzLTEyLTAzICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg T2JqZWN0QWxsb2NhdGlvblByb2ZpbGUgaXMgcmFjeSBhbmQgdGhlIERGRyBzaG91bGQgYmUgY29v bCB3aXRoIHRoYXQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTEyNTE3MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorICAgICAg ICAKKyAgICAgICAgV2Ugd291bGQgcHJldmlvdXNseSBzb21ldGltZXMgZ2V0IGEgbnVsbCBTdHJ1 Y3R1cmUgYmVjYXVzZSBjaGVja2luZyBpZiB0aGUgcHJvZmlsZSBpcyBub24tbnVsbCBhbmQgbG9h ZGluZworICAgICAgICB0aGUgc3RydWN0dXJlIGZyb20gaXQgd2VyZSB0d28gc2VwYXJhdGUgb3Bl cmF0aW9ucy4KKworICAgICAgICAqIGRmZy9ERkdBYnN0cmFjdEludGVycHJldGVySW5saW5lcy5o OgorICAgICAgICAoSlNDOjpERkc6Ojo6ZXhlY3V0ZUVmZmVjdHMpOgorICAgICAgICAqIGRmZy9E RkdBYnN0cmFjdFZhbHVlLmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjpBYnN0cmFjdFZhbHVlOjpz ZXRGdXR1cmVQb3NzaWJsZVN0cnVjdHVyZSk6CisgICAgICAgICogZGZnL0RGR0J5dGVDb2RlUGFy c2VyLmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjpCeXRlQ29kZVBhcnNlcjo6cGFyc2VCbG9jayk6 CisgICAgICAgICogcnVudGltZS9KU0Z1bmN0aW9uLmg6CisgICAgICAgIChKU0M6OkpTRnVuY3Rp b246OmFsbG9jYXRpb25Qcm9maWxlKToKKyAgICAgICAgKEpTQzo6SlNGdW5jdGlvbjo6YWxsb2Nh dGlvblN0cnVjdHVyZSk6CisKIDIwMTMtMTItMDMgIHBlYXZvQG91dGxvb2suY29tICA8cGVhdm9A b3V0bG9vay5jb20+CiAKICAgICAgICAgdGVzdGFwaSB0ZXN0IGNyYXNoZXMgb24gV2luZG93cyBp biBXVEY6OlZlY3Rvcjx3Y2hhcl90LDY0LFdURjo6VW5zYWZlVmVjdG9yT3ZlcmZsb3c+OjpzaXpl KCkKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQWJzdHJhY3RJbnRlcnByZXRl cklubGluZXMuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0Fi c3RyYWN0SW50ZXJwcmV0ZXJJbmxpbmVzLmgJKHJldmlzaW9uIDE2MDAxMykKKysrIFNvdXJjZS9K YXZhU2NyaXB0Q29yZS9kZmcvREZHQWJzdHJhY3RJbnRlcnByZXRlcklubGluZXMuaAkod29ya2lu ZyBjb3B5KQpAQCAtMTEzNiw2ICsxMTM2LDcgQEAgYm9vbCBBYnN0cmFjdEludGVycHJldGVyPEFi c3RyYWN0U3RhdGVUeQogICAgICAgICBicmVhazsKIAogICAgIGNhc2UgTmV3T2JqZWN0OgorICAg ICAgICBBU1NFUlQobm9kZS0+c3RydWN0dXJlKCkpOwogICAgICAgICBmb3JOb2RlKG5vZGUpLnNl dChtX2dyYXBoLCBub2RlLT5zdHJ1Y3R1cmUoKSk7CiAgICAgICAgIG1fc3RhdGUuc2V0SGF2ZVN0 cnVjdHVyZXModHJ1ZSk7CiAgICAgICAgIGJyZWFrOwpJbmRleDogU291cmNlL0phdmFTY3JpcHRD b3JlL2RmZy9ERkdBYnN0cmFjdFZhbHVlLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNj cmlwdENvcmUvZGZnL0RGR0Fic3RyYWN0VmFsdWUuY3BwCShyZXZpc2lvbiAxNjAwMTMpCisrKyBT b3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0Fic3RyYWN0VmFsdWUuY3BwCSh3b3JraW5nIGNv cHkpCkBAIC0xNTMsNiArMTUzLDcgQEAgRmlsdHJhdGlvblJlc3VsdCBBYnN0cmFjdFZhbHVlOjpm aWx0ZXJCeQogCiB2b2lkIEFic3RyYWN0VmFsdWU6OnNldEZ1dHVyZVBvc3NpYmxlU3RydWN0dXJl KEdyYXBoJiBncmFwaCwgU3RydWN0dXJlKiBzdHJ1Y3R1cmUpCiB7CisgICAgQVNTRVJUKHN0cnVj dHVyZSk7CiAgICAgaWYgKGdyYXBoLndhdGNocG9pbnRzKCkuaXNTdGlsbFZhbGlkKHN0cnVjdHVy ZS0+dHJhbnNpdGlvbldhdGNocG9pbnRTZXQoKSkpCiAgICAgICAgIG1fZnV0dXJlUG9zc2libGVT dHJ1Y3R1cmUgPSBzdHJ1Y3R1cmU7CiAgICAgZWxzZQpJbmRleDogU291cmNlL0phdmFTY3JpcHRD b3JlL2RmZy9ERkdCeXRlQ29kZVBhcnNlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFT Y3JpcHRDb3JlL2RmZy9ERkdCeXRlQ29kZVBhcnNlci5jcHAJKHJldmlzaW9uIDE2MDAxMykKKysr IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQnl0ZUNvZGVQYXJzZXIuY3BwCSh3b3JraW5n IGNvcHkpCkBAIC0xOTE2LDE5ICsxOTE2LDE4IEBAIGJvb2wgQnl0ZUNvZGVQYXJzZXI6OnBhcnNl QmxvY2sodW5zaWduZWQKICAgICAgICAgICAgICAgICBBU1NFUlQoY2VsbC0+aW5oZXJpdHMoSlNG dW5jdGlvbjo6aW5mbygpKSk7CiAgICAgICAgICAgICAgICAgCiAgICAgICAgICAgICAgICAgSlNG dW5jdGlvbiogZnVuY3Rpb24gPSBqc0Nhc3Q8SlNGdW5jdGlvbio+KGNlbGwpOwotICAgICAgICAg ICAgICAgIE9iamVjdEFsbG9jYXRpb25Qcm9maWxlKiBhbGxvY2F0aW9uUHJvZmlsZSA9IGZ1bmN0 aW9uLT50cnlHZXRBbGxvY2F0aW9uUHJvZmlsZSgpOwotICAgICAgICAgICAgICAgIGlmIChhbGxv Y2F0aW9uUHJvZmlsZSkgeworICAgICAgICAgICAgICAgIGlmIChTdHJ1Y3R1cmUqIHN0cnVjdHVy ZSA9IGZ1bmN0aW9uLT5hbGxvY2F0aW9uU3RydWN0dXJlKCkpIHsKICAgICAgICAgICAgICAgICAg ICAgYWRkVG9HcmFwaChBbGxvY2F0aW9uUHJvZmlsZVdhdGNocG9pbnQsIE9wSW5mbyhmdW5jdGlv bikpOwogICAgICAgICAgICAgICAgICAgICAvLyBUaGUgY2FsbGVlIGlzIHN0aWxsIGxpdmUgdXAg dG8gdGhpcyBwb2ludC4KICAgICAgICAgICAgICAgICAgICAgYWRkVG9HcmFwaChQaGFudG9tLCBj YWxsZWUpOwotICAgICAgICAgICAgICAgICAgICBzZXQoVmlydHVhbFJlZ2lzdGVyKGN1cnJlbnRJ bnN0cnVjdGlvblsxXS51Lm9wZXJhbmQpLAotICAgICAgICAgICAgICAgICAgICAgICAgYWRkVG9H cmFwaChOZXdPYmplY3QsIE9wSW5mbyhhbGxvY2F0aW9uUHJvZmlsZS0+c3RydWN0dXJlKCkpKSk7 CisgICAgICAgICAgICAgICAgICAgIHNldChWaXJ0dWFsUmVnaXN0ZXIoY3VycmVudEluc3RydWN0 aW9uWzFdLnUub3BlcmFuZCksIGFkZFRvR3JhcGgoTmV3T2JqZWN0LCBPcEluZm8oc3RydWN0dXJl KSkpOwogICAgICAgICAgICAgICAgICAgICBhbHJlYWR5RW1pdHRlZCA9IHRydWU7CiAgICAgICAg ICAgICAgICAgfQogICAgICAgICAgICAgfQotICAgICAgICAgICAgaWYgKCFhbHJlYWR5RW1pdHRl ZCkKKyAgICAgICAgICAgIGlmICghYWxyZWFkeUVtaXR0ZWQpIHsKICAgICAgICAgICAgICAgICBz ZXQoVmlydHVhbFJlZ2lzdGVyKGN1cnJlbnRJbnN0cnVjdGlvblsxXS51Lm9wZXJhbmQpLAogICAg ICAgICAgICAgICAgICAgICBhZGRUb0dyYXBoKENyZWF0ZVRoaXMsIE9wSW5mbyhjdXJyZW50SW5z dHJ1Y3Rpb25bM10udS5vcGVyYW5kKSwgY2FsbGVlKSk7CisgICAgICAgICAgICB9CiAgICAgICAg ICAgICBORVhUX09QQ09ERShvcF9jcmVhdGVfdGhpcyk7CiAgICAgICAgIH0KIApJbmRleDogU291 cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNGdW5jdGlvbi5oCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNv dXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTRnVuY3Rpb24uaAkocmV2aXNpb24gMTYwMDEz KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNGdW5jdGlvbi5oCSh3b3JraW5n IGNvcHkpCkBAIC0xMzcsMjIgKzEzNyw5IEBAIG5hbWVzcGFjZSBKU0MgewogICAgICAgICAgICAg ICAgIHJldHVybiBjcmVhdGVBbGxvY2F0aW9uUHJvZmlsZShleGVjLCBpbmxpbmVDYXBhY2l0eSk7 CiAgICAgICAgICAgICByZXR1cm4gJm1fYWxsb2NhdGlvblByb2ZpbGU7CiAgICAgICAgIH0KLQot ICAgICAgICBPYmplY3RBbGxvY2F0aW9uUHJvZmlsZSogdHJ5R2V0QWxsb2NhdGlvblByb2ZpbGUo KQotICAgICAgICB7Ci0gICAgICAgICAgICBpZiAobV9hbGxvY2F0aW9uUHJvZmlsZS5pc051bGwo KSkKLSAgICAgICAgICAgICAgICByZXR1cm4gMDsKLSAgICAgICAgICAgIGlmIChtX2FsbG9jYXRp b25Qcm9maWxlV2F0Y2hwb2ludC5oYXNCZWVuSW52YWxpZGF0ZWQoKSkKLSAgICAgICAgICAgICAg ICByZXR1cm4gMDsKLSAgICAgICAgICAgIHJldHVybiAmbV9hbGxvY2F0aW9uUHJvZmlsZTsKLSAg ICAgICAgfQotICAgICAgICAKLSAgICAgICAgdm9pZCBhZGRBbGxvY2F0aW9uUHJvZmlsZVdhdGNo cG9pbnQoV2F0Y2hwb2ludCogd2F0Y2hwb2ludCkKLSAgICAgICAgewotICAgICAgICAgICAgQVNT RVJUKHRyeUdldEFsbG9jYXRpb25Qcm9maWxlKCkpOwotICAgICAgICAgICAgbV9hbGxvY2F0aW9u UHJvZmlsZVdhdGNocG9pbnQuYWRkKHdhdGNocG9pbnQpOwotICAgICAgICB9CiAgICAgICAgIAor ICAgICAgICBTdHJ1Y3R1cmUqIGFsbG9jYXRpb25TdHJ1Y3R1cmUoKSB7IHJldHVybiBtX2FsbG9j YXRpb25Qcm9maWxlLnN0cnVjdHVyZSgpOyB9CisKICAgICAgICAgSW5saW5lV2F0Y2hwb2ludFNl dCYgYWxsb2NhdGlvblByb2ZpbGVXYXRjaHBvaW50U2V0KCkKICAgICAgICAgewogICAgICAgICAg ICAgcmV0dXJuIG1fYWxsb2NhdGlvblByb2ZpbGVXYXRjaHBvaW50Owo=