RESOLVED FIXED 176041
Web Inspector: Optimize View.prototype.removeSubview
https://bugs.webkit.org/show_bug.cgi?id=176041
Summary Web Inspector: Optimize View.prototype.removeSubview
Nikita Vasilyev
Reported 2017-08-28 17:56:25 PDT
removeSubview looks up a value in an array twice: 1. this._subviews.includes(view) 2. this._subviews.remove(view, true), which loops over an array https://github.com/WebKit/webkit/blob/0b21e4ce918800f832d96011b4ac695024c7882e/Source/WebInspectorUI/UserInterface/Views/View.js#L118
Attachments
Patch (2.42 KB, patch)
2017-08-28 18:01 PDT, Nikita Vasilyev
no flags
Patch (5.21 KB, patch)
2017-08-28 19:11 PDT, Nikita Vasilyev
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-elcapitan (1.41 MB, application/zip)
2017-08-28 20:16 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.51 MB, application/zip)
2017-08-28 20:21 PDT, Build Bot
no flags
Archive of layout-test-results from ews112 for mac-elcapitan (2.17 MB, application/zip)
2017-08-28 20:37 PDT, Build Bot
no flags
Patch (9.13 KB, patch)
2017-08-29 11:15 PDT, Nikita Vasilyev
mattbaker: review+
Patch (9.09 KB, patch)
2017-08-29 11:44 PDT, Nikita Vasilyev
no flags
Nikita Vasilyev
Comment 1 2017-08-28 18:01:06 PDT
Matt Baker
Comment 2 2017-08-28 18:39:39 PDT
Comment on attachment 319223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319223&action=review > Source/WebInspectorUI/ChangeLog:12 > + Since we loop backwards, the argument should be onlyLast. Wow, pretty big oversight! I think the places where onlyFirst/onlyLast was set to true are all arrays of unique items, so it probably doesn't matter. In fact the only place where we actually depend on it being false is in ContentViewContainer.prototype._clearTombstonesForContentView. In any case, whether it is the first or last item that is removed is an implementation detail. This function has a confusing API, and should probably be split into remove and removeAll. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:497 > + value: function(value, onlyLast) One downside to naming parameters passed to functions is that they need to be fixed up when making changes like this: .../OpenSource/Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js: 875 const onlyFirst = true; 876 for (let invalidAction of invalidActions) 877: options.actions.remove(invalidAction, onlyFirst); 878 879 return options; .../OpenSource/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js: 387 let tombstoneContentViewContainers = this._tombstoneContentViewContainersForContentView(contentView); 388 const onlyFirst = false; 389: tombstoneContentViewContainers.remove(this, onlyFirst); 390 391 for (let entry of this._backForwardList) { ... 405 let tombstoneContentViewContainers = this._tombstoneContentViewContainersForContentView(contentView); 406 const onlyFirst = true; 407: tombstoneContentViewContainers.remove(this, onlyFirst); 408 return; 409 } > Source/WebInspectorUI/UserInterface/Views/View.js:124 > + Style: remove extra newline.
Nikita Vasilyev
Comment 3 2017-08-28 18:49:05 PDT
Comment on attachment 319223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319223&action=review >> Source/WebInspectorUI/ChangeLog:12 >> + Since we loop backwards, the argument should be onlyLast. > > Wow, pretty big oversight! I think the places where onlyFirst/onlyLast was set to true are all arrays of unique items, so it probably doesn't matter. In fact the only place where we actually depend on it being false is in ContentViewContainer.prototype._clearTombstonesForContentView. In any case, whether it is the first or last item that is removed is an implementation detail. This function has a confusing API, and should probably be split into remove and removeAll. If we split it into remove and removeAll, what would remove do? Remove onlyLast?
Nikita Vasilyev
Comment 4 2017-08-28 19:11:03 PDT
Matt Baker
Comment 5 2017-08-28 20:10:13 PDT
Comment on attachment 319228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319228&action=review > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:388 > + tombstoneContentViewContainers.remove(this); `onlyFirst` was false in this case, so it should be `removeAll`.
Build Bot
Comment 6 2017-08-28 20:16:33 PDT
Comment on attachment 319228 [details] Patch Attachment 319228 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4400803 New failing tests: inspector/unit-tests/array-utilities.html
Build Bot
Comment 7 2017-08-28 20:16:34 PDT
Created attachment 319234 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Build Bot
Comment 8 2017-08-28 20:21:46 PDT
Comment on attachment 319228 [details] Patch Attachment 319228 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4400811 New failing tests: inspector/unit-tests/array-utilities.html
Build Bot
Comment 9 2017-08-28 20:21:48 PDT
Created attachment 319235 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Build Bot
Comment 10 2017-08-28 20:37:32 PDT
Comment on attachment 319228 [details] Patch Attachment 319228 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4400815 New failing tests: inspector/unit-tests/array-utilities.html
Build Bot
Comment 11 2017-08-28 20:37:33 PDT
Created attachment 319237 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Nikita Vasilyev
Comment 12 2017-08-29 11:15:41 PDT
Matt Baker
Comment 13 2017-08-29 11:25:14 PDT
Comment on attachment 319263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319263&action=review r=me, with a nit. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:499 > + for (let i = this.length - 1; i >= 0; --i) { Iterating from the end of the array feels weird now that it's no longer necessary. Plus the test output currently states that "remove should only remove the first matching value".
Joseph Pecoraro
Comment 14 2017-08-29 11:28:36 PDT
Comment on attachment 319263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319263&action=review > LayoutTests/inspector/unit-tests/array-utilities.html:133 > + InspectorTest.expectShallowEqual(arr1, [1, 2, 3], "remove should only remove the first matching value."); As Matt points out, this says it should remove the first but we see that the last was removed. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:497 > - value: function(value, onlyFirst) > + value: function(value) Nit: You can use the shorthand syntax here `value(value)` instead of `value: function(value)` > Source/WebInspectorUI/UserInterface/Base/Utilities.js:510 > + value: function(value) Nit: You can use the shorthand syntax here `value(value)` instead of `value: function(value)` > Source/WebInspectorUI/UserInterface/Views/View.js:129 > - this._subviews.remove(view, true); > + this._subviews.splice(index, 1); We could add an Array.prototype.removeAtIndex. Since I never know what splice does, removeAtIndex ends up reading clearer to me.
Nikita Vasilyev
Comment 15 2017-08-29 11:44:08 PDT
Nikita Vasilyev
Comment 16 2017-08-29 11:51:09 PDT
(In reply to Joseph Pecoraro from comment #14) > Comment on attachment 319263 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319263&action=review > > > LayoutTests/inspector/unit-tests/array-utilities.html:133 > > + InspectorTest.expectShallowEqual(arr1, [1, 2, 3], "remove should only remove the first matching value."); > > As Matt points out, this says it should remove the first but we see that the > last was removed. Makes sense, thanks! > > Source/WebInspectorUI/UserInterface/Views/View.js:129 > > - this._subviews.remove(view, true); > > + this._subviews.splice(index, 1); > > We could add an Array.prototype.removeAtIndex. Since I never know what > splice does, removeAtIndex ends up reading clearer to me. I don't know if I like adding a new method for this. I'd need to know of it existence to use it and I personally remember what splice does already.
Nikita Vasilyev
Comment 17 2017-08-29 11:54:09 PDT
Comment on attachment 319263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319263&action=review >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:510 >> + value: function(value) > > Nit: You can use the shorthand syntax here `value(value)` instead of `value: function(value)` Most methods in this file still use the old syntax. I'm going to convert all of them to the shorthand one.
WebKit Commit Bot
Comment 18 2017-08-29 12:26:00 PDT
Comment on attachment 319267 [details] Patch Clearing flags on attachment: 319267 Committed r221308: <http://trac.webkit.org/changeset/221308>
WebKit Commit Bot
Comment 19 2017-08-29 12:26:02 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 20 2017-08-29 12:26:59 PDT
Nikita Vasilyev
Comment 21 2017-08-29 14:54:43 PDT
(In reply to Nikita Vasilyev from comment #16) > > > Source/WebInspectorUI/UserInterface/Views/View.js:129 > > > - this._subviews.remove(view, true); > > > + this._subviews.splice(index, 1); > > > > We could add an Array.prototype.removeAtIndex. Since I never know what > > splice does, removeAtIndex ends up reading clearer to me. > > I don't know if I like adding a new method for this. I'd need to know of it > existence to use it and I personally remember what splice does already. On the other hand, we have Array.prototyte.insertAtIndex already. I don't mind if somebody adds Array.prototype.removeAtIndex.
Note You need to log in before you can comment on or make changes to this bug.