Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(172)

Issue 29804575: Noissue - Remove unnecessary typeof checks in lib/matcher.js (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months ago by Manish Jethani
Modified:
1 month, 2 weeks ago
Reviewers:
sergei
CC:
kzar
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

In many even if not most cases, `typeof foo == "undefined"` can be replaced with a `!foo` or `foo == null`, both of which are faster.

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -5 lines) Patch
M lib/matcher.js View 4 chunks +5 lines, -5 lines 1 comment Download

Messages

Total messages: 3
Manish Jethani
2 months ago (2018-06-12 04:47:09 UTC) #1
Manish Jethani
Patch Set 1 https://codereview.adblockplus.org/29804575/diff/29804576/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29804575/diff/29804576/lib/matcher.js#newcode87 lib/matcher.js:87: if (keyword == null) keyword can ...
2 months ago (2018-06-12 04:50:27 UTC) #2
Manish Jethani
2 months ago (2018-06-13 06:26:51 UTC) #3
I ran this test on Node.js v8.9.1, Chrome Canary (69), and Firefox 59:

  (function()
  {
    let array = new Array(10000000);
    for (let i = 0; i < 10000000; i++)
      array[i] = Math.random() < .5 ? {} : undefined;

    let s = Date.now();
    let n = 0;
    for (let i = 0; i < 10000000; i++)
      if (typeof array[i] != "undefined")
      //if (array[i] != null)
        n++;
    console.log(Date.now() - s, n);
  })();

The performance is about the same for `typeof array[i] != "undefined"` vs.
`array[i] != null` on both Chrome and Firefox. On Node.js v8.9.1 (V8
6.1.534.47), the latter is only slightly faster (~1-4%). So it looks like this
doesn't matter for browser-based stuff.

Sergei, if you think this helps for libadblockplus, we can make the change, but
otherwise I'll close this.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5