In response to a reviver function's result, JSON.parse modifies an object property using the CreateDataProperty spec function. CreateDataProperty is specified to fail if the property is non-configurable. Hence, JSON.parse should not modify non-configurable properties.
Created attachment 373115 [details] Patch
Comment on attachment 373115 [details] Patch Attachment 373115 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12603230 New failing tests: js/dom/JSON-parse.html
Created attachment 373116 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 373115 [details] Patch Attachment 373115 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12603280 New failing tests: js/dom/JSON-parse.html
Created attachment 373119 [details] Archive of layout-test-results from ews106 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 373115 [details] Patch Attachment 373115 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12603304 New failing tests: js/dom/JSON-parse.html
Created attachment 373120 [details] Archive of layout-test-results from ews116 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 373115 [details] Patch Attachment 373115 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12603319 New failing tests: js/dom/JSON-parse.html
Created attachment 373121 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5
Comment on attachment 373115 [details] Patch Attachment 373115 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12603276 New failing tests: ChakraCore.yaml/ChakraCore/test/JSON/jx2.js.default apiTests
Comment on attachment 373115 [details] Patch Attachment 373115 [details] did not pass jsc-armv7-ews (jsc-only): Output: https://webkit-queues.webkit.org/results/12603273 New failing tests: ChakraCore.yaml/ChakraCore/test/JSON/jx2.js.default apiTests
Comment on attachment 373115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373115&action=review > Source/JavaScriptCore/runtime/JSONObject.cpp:716 > + else { > + PropertyDescriptor desc(filteredValue, static_cast<unsigned>(PropertyAttribute::None)); > + bool shouldThrow = false; > + array->defineOwnIndexedProperty(m_exec, indexStack.last(), desc, shouldThrow); > + } This looks very slow to me. Can we call this slow path only when the receiver callback is passed?
Comment on attachment 373115 [details] Patch Attachment 373115 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12603481 New failing tests: http/tests/css/filters-on-iframes-transform.html js/dom/JSON-parse.html
Created attachment 373128 [details] Archive of layout-test-results from ews211 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 373115 [details] Patch Attachment 373115 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12604786 New failing tests: js/dom/JSON-parse.html
Created attachment 373136 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 373178 [details] Patch Fix test and clarify comments.
Created attachment 373180 [details] Patch Adjust expectations for ChakraCore test.
(In reply to Yusuke Suzuki from comment #12) > Comment on attachment 373115 [details] > Patch > > This looks very slow to me. Can we call this slow path only when the > receiver callback is passed? This method (Walker::walk) is invoked only if receiver is a function. Also, JSObject::defineOwnIndexedProperty has fast path to putDirectIndex, which will always be taken for regular arrays. There is another patch for this method where I preserved fast path for regular arrays: https://bugs.webkit.org/show_bug.cgi?id=199292. I would really appreaciate your feedback on it. --- Tests were failing because [[DefineOwnProperty]] changes enumeration order for existing property if given descriptor is different: ``` let obj = {a: 1, b: 2} Object.defineProperty(obj, "a", {value: 3}) Object.keys(obj) // => ["b", "a"] ``` Please see https://bugs.webkit.org/show_bug.cgi?id=142933.
Comment on attachment 373180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373180&action=review I didn't fully review this, and someone with more JSC expertise will probably review, but I did have a few comments: To me this looks like this might have a performance effect. Can someone help us check JSON parsing performance to make sure it’s either faster or not measurably slower? > JSTests/ChangeLog:8 > + * ChakraCore/test/JSON/jx2.baseline-jsc: Adjust expectations. *Why* do the expectations change? It looks like orders of properties are changing? Why is that happening? Is it actively better or just OK? Why is it OK? If this is an accident, then: Is there something the authors of the test can do to make it less dependent on engine internals? Should we be doing something in our engine so serialization writes in a predictable order instead of what seems like arbitrary order? > Source/JavaScriptCore/runtime/JSONObject.cpp:713 > + PropertyDescriptor desc(filteredValue, static_cast<unsigned>(PropertyAttribute::None)); While we sometimes use abbreviations like "desc", generally WebKit project coding style encourages use of words like "description" instead. > Source/JavaScriptCore/runtime/JSObject.h:213 > + // This is pretty much like performing defineOwnProperty, but without extensibility check and property descriptor validation. Why the words "pretty much"? Is there something not mentioned here? The comment would be better without the vagueness of "pretty much". Could the comment be: // This is like defineOwnProperty without an extensibility check or property descriptor validation. Or is that inaccurate in some way? > Source/JavaScriptCore/runtime/JSObject.h:242 > + // This is pretty much like performing defineOwnProperty(propertyName, {configurable:true, writable:true, enumerable:true, value:value}), > + // but without extensibility check and property descriptor validation. Ditto.
Created attachment 373199 [details] Patch Rename variables and remove comments.
(In reply to Darin Adler from comment #20) > Comment on attachment 373180 [details] > Patch > > To me this looks like this might have a performance effect. For arrays, we are still on the fast path: this change won't cause any noticable regressions. For objects though, this patch adds a few descriptor checks, nothing particularly slow. We can skip calling [[DefineOwnProperty]] if returned value from reviver is the same and holder is not a Proxy. V8 invokes [[DefineOwnProperty]] without extra checks though (for both arrays and objects). > > JSTests/ChangeLog:8 > > + * ChakraCore/test/JSON/jx2.baseline-jsc: Adjust expectations. > > *Why* do the expectations change? It looks like orders of properties are > changing? Why is that happening? Is it actively better or just OK? Why is it > OK? > > If this is an accident, then: Is there something the authors of the test can > do to make it less dependent on engine internals? Should we be doing > something in our engine so serialization writes in a predictable order > instead of what seems like arbitrary order? Serialization is perfect: property order is correct if `reviver` is not provided. Order has changed because JSObject::defineOwnProperty removes old descriptor and inserts new one in new place (instead of overwriting and keeping the order). It doesn't corrupt property order if `reviver` returns `value` (second argument), which is the most common case I've seen in the wild. I've made js/dom/JSON-parse.html independent of this. ChakraCore test relies on `reviver` returning another value. Please see comment #19. > > > Source/JavaScriptCore/runtime/JSObject.h:213 > > + // This is pretty much like performing defineOwnProperty, but without extensibility check and property descriptor validation. > > Why the words "pretty much"? Is there something not mentioned here? `defineOwnProperty` is actually performed in full, with all the checks, but only if `canDoFastPutDirectIndex` returns `false`. `canDoFastPutDirectIndex` is a bit complex to describe it in comment, so I removed references to `defineOwnProperty` from comments.
(In reply to Alexey Shvayka from comment #22) > For arrays, we are still on the fast path: this change won't cause any > noticable regressions. Normally we like to prove that through some quick performance testing rather than just relying on logic. > For objects though, this patch adds a few descriptor checks, nothing > particularly slow. > > We can skip calling [[DefineOwnProperty]] if returned value from reviver is > the same and holder is not a Proxy. > V8 invokes [[DefineOwnProperty]] without extra checks though (for both > arrays and objects). Again, I think we can define "fast enough" by running some performance tests and making sure it's not something measurable. > Order has changed because JSObject::defineOwnProperty removes old descriptor > and inserts new one in new place (instead of overwriting and keeping the > order). Thanks. That's good new. I suggest the reason for the change in the test results be mentioned in the change log. > `canDoFastPutDirectIndex` is a bit complex to describe it in comment, so I > removed references to `defineOwnProperty` from comments. I’m a little sad to hear that. I like the idea of having a comment, but if we can’t find something that is simple and clear to say, I understand making the choice to remove the comment instead.
Created attachment 373595 [details] Patch Add microbenchmarks and note on changed test results.
(In reply to Darin Adler from comment #23) > (In reply to Alexey Shvayka from comment #22) > > For arrays, we are still on the fast path: this change won't cause any > > noticable regressions. > > Normally we like to prove that through some quick performance testing rather > than just relying on logic. > I've failed to find any usages of `reviver` in mainstream benchmarks, so I've added a few microbenchmarks. Results of 10 runs x 4 samples: json-parse-array-reviver: 235.92ms trunk, 240.70ms patch (+2%) json-parse-array-reviver-same-value: 164.82ms trunk, 170.60ms patch (+3.5%) json-parse-object-reviver: 256.48ms trunk, 539.20ms patch (+110%) json-parse-object-reviver-same-value: 227.29ms trunk, 240.44ms patch (+5.8%) As it was expected, common cases (*-same-value) are fine, but result of `json-parse-object-reviver` is pretty disturbing. I think we should fix https://bugs.webkit.org/show_bug.cgi?id=142933 first and measure performance again.
(In reply to Alexey Shvayka from comment #25) > (In reply to Darin Adler from comment #23) > > (In reply to Alexey Shvayka from comment #22) > > > For arrays, we are still on the fast path: this change won't cause any > > > noticable regressions. > > > > Normally we like to prove that through some quick performance testing rather > > than just relying on logic. > > > > I've failed to find any usages of `reviver` in mainstream benchmarks, so > I've added a few microbenchmarks. Results of 10 runs x 4 samples: > json-parse-array-reviver: 235.92ms trunk, 240.70ms patch (+2%) > json-parse-array-reviver-same-value: 164.82ms trunk, 170.60ms patch (+3.5%) > json-parse-object-reviver: 256.48ms trunk, 539.20ms patch (+110%) > json-parse-object-reviver-same-value: 227.29ms trunk, 240.44ms patch > (+5.8%) > > As it was expected, common cases (*-same-value) are fine, but result of > `json-parse-object-reviver` is pretty disturbing. > I think we should fix https://bugs.webkit.org/show_bug.cgi?id=142933 first > and measure performance again. Can you measure the perf again because the reordering patch is landed :) ?
Created attachment 404651 [details] Patch
(In reply to Alexey Shvayka from comment #27) > Created attachment 404651 [details] > Patch Warmed-up runs, --outer 50: r264388 patch json-parse-object-reviver-same-value 241.8507+-4.5414 ^ 213.8166+-4.2992 ^ definitely 1.1311x faster json-parse-object-reviver 270.6204+-4.2983 ^ 239.5138+-4.4341 ^ definitely 1.1299x faster json-parse-array-reviver-same-value 168.0164+-3.1485 ? 169.7259+-2.2783 ? might be 1.0102x slower json-parse-array-reviver 223.8018+-4.1355 ? 229.6355+-3.6125 ? might be 1.0261x slower <geometric> 222.3860+-2.0189 ^ 211.0908+-1.9662 ^ definitely 1.0535x faster (In reply to Yusuke Suzuki from comment #26) > Can you measure the perf again because the reordering patch is landed :) ? Without the fast path, it is 20% regression for json-parse-object-reviver.js.
(In reply to Darin Adler from comment #23) > > `canDoFastPutDirectIndex` is a bit complex to describe it in comment, so I > > removed references to `defineOwnProperty` from comments. > > I’m a little sad to hear that. I like the idea of having a comment, but if > we can’t find something that is simple and clear to say, I understand making > the choice to remove the comment instead. r258170 fixed putDirectIndex() to perform [[DefineOwnProperty]], so I've reverted the comment change.
Should I be worried about the json-parse-array-reviver slow-down?
Committed r264833: <https://trac.webkit.org/changeset/264833> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404651 [details].
<rdar://problem/66058601>
(In reply to Darin Adler from comment #30) > Should I be worried about the json-parse-array-reviver slow-down? I don't think so: slow path this patch adds isn't executed by these microbenchmarks. Retested with the same params: r264603 patch json-parse-array-reviver-same-value 160.5339+-3.6430 ? 161.9704+-2.7285 ? json-parse-array-reviver 221.6459+-3.4088 217.0408+-4.2595 might be 1.0212x faster <geometric> 188.4357+-2.7034 187.2907+-2.3668 might be 1.0061x faster Results are almost always within error margin.