Issue 29895567: Noissue - Avoid access to stack trace unless necessary (Closed)
|
Created:
Sept. 29, 2018, 1:37 p.m. by Manish Jethani Modified:
Sept. 29, 2018, 4:59 p.m. Reviewers:
Sebastian Noack Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionNoissue - Avoid access to stack trace unless necessary
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move declaration #
Total comments: 2
MessagesTotal messages: 7
Patch Set 1 Profiling the extension, some odd things stand out. One of these things is the stack getter. It takes up only a small percentage of the extension's execution time (0.5% when called from here), but given that we don't even use this object for cases where the last argument is a function (which is still most cases as of now), at least we can avoid accessing it here.
It feels a little backwards to me to optimize for a use case that we want to get rid of. Note that we only support passing in a callback rather then relying on the returned promise for compatible with legacy code. But well, considering that this change doesn't add any additional complexity, fine with me. https://codereview.adblockplus.org/29895567/diff/29895568/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29895567/diff/29895568/polyfill.js#newcode95 polyfill.js:95: let callStack = new Error().stack; Nit: Perhaps initialize this variable along the resolvePromise and rejectPromise variables below.
Patch Set 2: Move declaration On 2018/09/29 13:53:21, Sebastian Noack wrote: > It feels a little backwards to me to optimize for a use case that we want to get > rid of. Note that we only support passing in a callback rather then relying on > the returned promise for compatible with legacy code. But well, considering that > this change doesn't add any additional complexity, fine with me. Yeah, we should evaluate whether it makes sense to move to the promise-based API exclusively. If we keep it partial like this, we should _not_ wrap certain APIs on Chrome. There are APIs being wrapped but not taking advantage of the wrapping (still passing in callbacks), in which case the wrapper is just unnecessary overhead. Message passing in the extension seems to take up a lot of the processing work, so maybe those APIs should remain unwrapped. https://codereview.adblockplus.org/29895567/diff/29895568/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29895567/diff/29895568/polyfill.js#newcode95 polyfill.js:95: let callStack = new Error().stack; On 2018/09/29 13:53:21, Sebastian Noack wrote: > Nit: Perhaps initialize this variable along the resolvePromise and rejectPromise > variables below. Done.
On 2018/09/29 15:49:11, Manish Jethani wrote: > Patch Set 2: Move declaration > > On 2018/09/29 13:53:21, Sebastian Noack wrote: > > It feels a little backwards to me to optimize for a use case that we want to > get > > rid of. Note that we only support passing in a callback rather then relying on > > the returned promise for compatible with legacy code. But well, considering > that > > this change doesn't add any additional complexity, fine with me. > > Yeah, we should evaluate whether it makes sense to move to the promise-based API > exclusively. That is the plan. > If we keep it partial like this, we should _not_ wrap certain APIs > on Chrome. There are APIs being wrapped but not taking advantage of the wrapping > (still passing in callbacks), in which case the wrapper is just unnecessary > overhead. > > Message passing in the extension seems to take up a lot of the processing work, > so maybe those APIs should remain unwrapped. I still have hopes that Chromium catches up, and will eventually support promises natively. FWIW, the respective Chromium bug has been accepted: https://bugs.chromium.org/p/chromium/issues/detail?id=328932 For the time being, I rather not encourage using callbacks, for some micro optimization. However, the saving the stack trace adds significant overhead, we might consider stop doing that. https://codereview.adblockplus.org/29895567/diff/29895570/polyfill.js File polyfill.js (left): https://codereview.adblockplus.org/29895567/diff/29895570/polyfill.js#oldcode107 polyfill.js:107: Nit: This blank line seems redundant.
On 2018/09/29 16:07:14, Sebastian Noack wrote: > On 2018/09/29 15:49:11, Manish Jethani wrote: > > For the time being, I rather not encourage using callbacks, for some micro > optimization. > > However, the saving the stack trace adds significant overhead, we might consider > stop doing that. I have never _found_ it useful to have this stack trace, but maybe because it's so subtle I may have been benefiting from it all along without realizing that it's not the real stack trace but the fake one we create before making the call. Anyway, I don't really have an opinion on whether to keep it. https://codereview.adblockplus.org/29895567/diff/29895570/polyfill.js File polyfill.js (left): https://codereview.adblockplus.org/29895567/diff/29895570/polyfill.js#oldcode107 polyfill.js:107: On 2018/09/29 16:07:14, Sebastian Noack wrote: > Nit: This blank line seems redundant. I'd prefer to keep the variable standing out from the rest as it is clearly only for debugging purposes. It's easier to notice this way.
LGTM 
|
