Use nullIterable instead of new Set in RuleSets.ruleCache by cschanaj · Pull Request #12577 · EFForg/https-everywhere

cschanaj · 2017-09-15T13:41:55Z

@cowlicks this replaces and closes #12494. thanks!

P.S. eslint error is unexpected, but it should be fine to fix it here.

ghost · 2017-09-15T14:13:00Z

+const nullIterable = Object.create(null, {
+  [Symbol.iterator]: {
+    value: function* () {
+      // do nothing


This can be done in a simpler way.

const nullIterator = { next() { return { done: true }; } }; const nullIterable = { [Symbol.iterator]: function () { return nullIterator; } };

Will also save memory since it will not create a new iterator each time.

Having just one name is nice. How about just:

const nullIterable = Object.create(null, { [Symbol.iterator]: { value: () => ({ next() { return {done: true}; } }) } });

@cowlicks const nullIterable = (function* () {})() would be better.

@cowlicks why use Object.create(null, { ...?

@cowlicks This will create a new instance of { next() { return { done: true } } } each time.

Currently I have this code:

const nullIterator = { next() { return { done: true }; } }; const nullIterable = { size: 0, [Symbol.iterator]: function () { return nullIterator; } };

@koops76 see #12577 (comment)

re: new instance of ... thing - that is fine, it should get GC'd immediately anyway

ghost · 2017-09-15T14:53:36Z

+};
+
+const nullIterable = {
+  [Symbol.iterator]: function *() {


Simply change it into () => nullIterator I guess.

without * it does not work for me (it is a generator function).

See also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Iterators_and_Generators

@cschanaj See #12577 (comment).

Remove the * after fixing this issue.

Remove the * now.

ghost · 2017-09-15T15:12:34Z


+// Empty iterable singleton to reduce memory usage (#12494).
+const nullIterator = {
+  next : () => { done: true }


next: () => ({ done: true })

or next() { return { done: true } }

ghost · 2017-09-15T15:26:54Z

(function* () {})() is the shortest code for empty Iterable. Use it.

ghost · 2017-09-15T18:47:23Z

+  next : () => ({ done: true })
+};
+
+const nullIterable = {


Simply replace this entire code with const nullIterable = (function* () {})(). Calling a generator function will return a Generator instance which implements both Iterator and Iterable.

I choose to create Object.creat(null, ... because the created object wouldn't have extra properties, so it would be harder to misuse. This seemed important because we are using it alongside real data.

Also, using Object.create(... lets us specify other properties we might want, like size in this case.

Would simply doing { size: 0 } cause a conflict?

Ah, it makes it read only.

cowlicks · 2017-09-15T20:59:20Z

-      log(DBUG, "Ruleset cache hit for " + host + " items:" + cached_item.length);
+    if (this.ruleCache.has(host)) {
+      let cached_item = this.ruleCache.get(host);
+      let len = cached_item && cached_item != nullIterable ? cached_item.size : 0;


just add a size property to the nullIterable thing.

ghost · 2017-09-15T21:28:45Z

@cowlicks Look at this code:

const nullIterator = Object.create(null, {
  next: { value: function() { return { done: true }; } }
});

const nullIterable = Object.create(null, {
  [Symbol.iterator]: { value: function() { return nullIterator; } }
});

Is it okay to use in your opinion?

cowlicks · 2017-09-15T21:55:12Z

      // Copy the host targets so we don't modify them.
-      results = results.concat(this.targets.get(host));
+      let results = this.targets.get(host);
+      results.map(result => resultSet.add(result));


I'm a little confused by having both results and resultsSet variables. You can just do:

if (this.targets.has(host)) { results = new Set([...results, this.targets.get(host)]); }

cowlicks · 2017-09-15T21:55:49Z

-    if (host.indexOf("..") != -1 || host.length > 255) {
-      log(WARN,"Malformed host passed to potentiallyApplicableRulesets: " + host);
-      return null;
+    if (host.length > 255 || host.indexOf("..") != -1) {


This makes sense 👍

cowlicks · 2017-09-15T21:58:38Z

    }

    // Replace each portion of the domain with a * in turn
-    var segmented = host.split(".");


Please avoid renaming and changing this code unless its necessary to the PR. This algorithm is important, but isn't well tested unfortunately.

cowlicks · 2017-09-15T22:14:41Z

-    for (let i=0; i < segmented.length; i++) {
-      let tmp = segmented[i];
-      segmented[i] = "*";
-      results = results.concat(this.targets.get(segmented.join(".")));


This could be something like my above comment, (https://github.com/EFForg/https-everywhere/pull/12577/files#r139258106) or maybe a ternary expression like

results = (this.targets.has(segmented.join('.')) ? (new Set([...results, ...this.targets.get(segmented.join('.')]) : results);

cowlicks · 2017-09-15T22:17:19Z

    // check *.z.google.com and *.google.com (we did *.y.z.google.com above)
-    for (var i = 2; i <= segmented.length - 2; ++i) {
-      var t = "*." + segmented.slice(i,segmented.length).join(".");
-      results = results.concat(this.targets.get(t));


Please revert this rename. Then just use one of the methods I suggested above to update the results

cowlicks · 2017-09-15T22:18:48Z


-    // Clean the results list, which may contain duplicates or undefined entries
-    var resultSet = new Set(results);
-    resultSet.delete(undefined);


Keep a results.delete(undefined) around to be safe.

cowlicks · 2017-09-15T22:36:52Z

@koops76 re: #12577 (comment)

It depends on the context. I'd prefer to avoid using 2 names here. I don't think it creating an iterator when iterated matters. It should be garbage collected immediately, and js engines are pretty good at optimizing simple functions like this.

@cschanaj with the size property it would look like:

const nullIterable = Object.create(null, {
  [Symbol.iterator]: {
    value: () => ({
      next() {
        return {done: true};
      }
    })
  },
  size: {value: 0},
});

cowlicks · 2017-09-15T23:00:45Z

@cschanaj in the future, please try to keep revisions to PR's as the same PR. This makes reviewing easier for me. It is hard to track what changed and what didn't between two similar PR's so I end up re-reading a lot of code. (I'm referring to this and #12494).

There is no need to worry about force pushes or anything.

cschanaj · 2017-09-16T02:24:27Z

@cowlicks modified accordingly, please check. thanks!!

cschanaj · 2017-09-16T05:41:20Z

@cowlicks Sorry for force pushing a few times. This PR is ready for review and merging now. thanks!

cschanaj · 2017-09-18T23:05:24Z

@cowlicks @Hainish I did not realize that you are modifying rules.js as well. I will rebase this branch in the weekend...

P.S. I have made a mistake in rebasing this. Please do not merge the current branch.

cowlicks · 2017-09-19T19:38:01Z

@cschanaj I can rebase this if you'd like.

cschanaj · 2017-09-20T12:16:19Z

thanks!!

cschanaj · 2017-09-20T23:09:44Z

@cowlicks please merge this if it looks good for you. thank you!!

cowlicks · 2017-09-22T21:22:32Z

+    } else {
+      util.log(DBUG, "Ruleset cache miss for " + host);
    }
-    util.log(util.DBUG, "Ruleset cache miss for " + host);


There are some issues with how this was merged, this line should have been kept (it should be util.DBUG).

cschanaj · 2017-09-23T06:06:07Z

@cowlicks Travis all passing now, please merge this if it looks good. (It has been sort of annoying to rebase a few time and keeping up with the incompatible changes).

cschanaj · 2017-09-28T22:10:48Z

@cowlicks could you please merge this before the next release?

cowlicks · 2017-10-03T21:29:52Z

@cschanaj sorry we won't have time! It will be in the next release. Totally my fault, I forgot to check back on this after you rebased :(

Thank you for your work!

cschanaj · 2017-10-06T23:54:42Z

@cowlicks Never mind. Please be sure to merge this PR before the next release. 😄

cowlicks · 2017-10-17T23:27:34Z

merged in #13208 thank you!

ghost reviewed Sep 15, 2017

View reviewed changes

cschanaj mentioned this pull request Sep 15, 2017

Fix WebExtension Excessive Memory Usage #12232

Closed

24 tasks

ghost reviewed Sep 15, 2017

View reviewed changes

cowlicks reviewed Sep 15, 2017

View reviewed changes

cowlicks mentioned this pull request Sep 16, 2017

Use null instead of new Set in RuleSets.ruleCache #12494

Closed

Use nullIterable instead of new Set in RuleSets.ruleCache

b019f9a

Merge branch 'master' into user-nullIterable

2c35305

cowlicks reviewed Sep 22, 2017

View reviewed changes

Chan Chak Shing added 2 commits September 23, 2017 13:36

Update rules.js

e488e15

Update rules.js

772f561

cschanaj mentioned this pull request Sep 29, 2017

Release 2017.10.[2|3] forthcoming #12827

Closed

cowlicks mentioned this pull request Oct 17, 2017

Rebase and test cschanaj's nullIterable PR #13208

Merged

cowlicks closed this Oct 17, 2017

cschanaj deleted the user-nullIterable branch October 24, 2017 11:03

Uh oh!

Conversation

cschanaj commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost Sep 15, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Sep 15, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Sep 15, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cschanaj Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Sep 15, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 15, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost Sep 15, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 15, 2017

Uh oh!

cowlicks Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cschanaj commented Sep 15, 2017 •

edited

Loading

ghost Sep 15, 2017 •

edited by ghost

Loading

ghost Sep 15, 2017 •

edited by ghost

Loading

ghost Sep 15, 2017 •

edited by ghost

Loading

cschanaj Sep 15, 2017 •

edited

Loading

ghost Sep 15, 2017 •

edited by ghost

Loading

ghost commented Sep 15, 2017 •

edited by ghost

Loading

ghost Sep 15, 2017 •

edited by ghost

Loading

cowlicks Sep 15, 2017 •

edited

Loading

cschanaj commented Sep 16, 2017 •

edited

Loading

cschanaj commented Sep 18, 2017 •

edited

Loading

cschanaj commented Sep 23, 2017 •

edited

Loading