Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: refactor to use iterable-weak-map #35915

Closed
wants to merge 10 commits into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Nov 1, 2020

Using an iterable WeakMap (a data-structure that uses WeakRef and
WeakMap), we are able to: stop relying on Module._cache to
serialize source maps; stop requiring an error object when calling
findSourceMap().

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 1, 2020

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. util Issues and PRs related to the built-in util module. labels Nov 1, 2020
class IterableWeakMap {
#weakMap = new WeakMap();
#refSet = new Set();
#finalizationGroup = new global.FinalizationRegistry(cleanup);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung @addaleax when I try to define FinalizationRegistry and WeakRef in primordials, I get a compilation error because the WeakRef and FinalizationRegistry are undefined at the time when primordials is initially loaded.

Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WeakRef and FinalizationRegistry cannot be used with snapshots because they can be disabled using a flag at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this means we can't use them for a utility like this?

We don't want source maps to stop working because the flag isn't set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a V8 --harmony flag, that's not supported and undocumented in Node.js. I'd say it's fine to use them.

lib/internal/util/iterable-weak-map.js Outdated Show resolved Hide resolved
lib/internal/util/iterable-weak-map.js Outdated Show resolved Hide resolved
lib/internal/util/iterable-weak-map.js Outdated Show resolved Hide resolved
lib/internal/source_map/source_map_cache.js Outdated Show resolved Hide resolved
lib/internal/source_map/source_map_cache.js Outdated Show resolved Hide resolved
#refSet = new Set();
#finalizationGroup = new global.FinalizationRegistry(cleanup);

set(key, value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how this works - this appears to create a new WeakRef each time the key is set even if the key "value" already exists in the map? Is this intentional?

Copy link
Member

@benjamingr benjamingr Nov 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var m = new Map();
var o = {};
var r1 = new WeakRef(o);
var r2 = new WeakRef(o);
m.set(r1, "foo");
console.log(m.get(r1)); // foo 
console.log(m.get(r2)); // undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this appears to create a new WeakRef each time the key is set even if the key "value" already exists in the map? Is this intentional

I'll add unit tests for the IterableWeakMap implementation, and will make sure this case is covered.

Copy link
Member

@benjamingr benjamingr Nov 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm saying - I don't understand how it works and to my understanding it's not supposed to. A test is fine, but a "this works because X" is also fine :]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjamingr I'm pretty sure you've identified a bug in the IterableWeakMap shared on the TC39 spec page -- we should only create a new WeakRef if there's not already an entry, otherwise we should perform an update.

Once I've fixed this, I'll share the updated code, and can provide a walk through of the logic if it's still confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, feel free to ping me when updated and I'll take a look :]

doc/api/module.md Outdated Show resolved Hide resolved
lib/internal/util/iterable-weak-map.js Outdated Show resolved Hide resolved
lib/internal/util/iterable-weak-map.js Outdated Show resolved Hide resolved
lib/internal/util/iterable-weak-map.js Outdated Show resolved Hide resolved
lib/internal/util/iterable-weak-map.js Outdated Show resolved Hide resolved
@bcoe
Copy link
Contributor Author

bcoe commented Nov 2, 2020

CC: @mcollina, I noticed a conversation you had on Twitter contemplating using IterableWeakMaps for a similar purpose.

class IterableWeakMap {
#weakMap = new SafeWeakMap();
#refSet = new SafeSet();
// TODO(aduh95): Add FinalizationRegistry to primordials
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to call it out explicitly: i think these primordials todos should be considered blockers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb @aduh95, how about the approach of exposing the makeSafe from primordials, and freezing WeakRef and FinalizationRegistry when iterable-weak-map.js is loaded:

// TODO(aduh95): Add WeakRef to primordials
const SafeWeakRef = makeSafe(
  globalThis.WeakRef,
  class SafeWeakRef extends globalThis.WeakRef {}
);

// This class is modified from the example code in the WeakRefs specification:
// https://github.com/tc39/proposal-weakrefs
// Licensed under ECMA's MIT-style license, see:
// https://github.com/tc39/ecma262/blob/master/LICENSE.md
class IterableWeakMap {
// ...
}}

source_map_cache.js pretty early on in the application lifecycle, _I think, before any userland code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems fine, but I’m not sure what’s wrong with eagerly freezing it, like all the other primordials?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the primordials are prepared, we only have access to a subset of builtins. V8 does not provide us the ones that can be disabled with flags by the user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhhh thanks, that makes sense.

@bcoe bcoe changed the title module: refactor to use iterable-weak-map errors: refactor to use iterable-weak-map Nov 3, 2020
@benjamingr
Copy link
Member

Generally look fine, lmk when it's ready and I'll check the branch out and play with it a bit :]

@bcoe bcoe changed the title errors: refactor to use iterable-weak-map module: refactor to use iterable-weak-map Nov 4, 2020
@bcoe bcoe marked this pull request as ready for review November 4, 2020 14:38
@bcoe bcoe requested a review from benjamingr November 4, 2020 14:39
@nodejs-github-bot

This comment has been minimized.

@bcoe bcoe requested a review from aduh95 November 4, 2020 14:39
@bcoe bcoe requested a review from aduh95 November 5, 2020 15:47
@bcoe bcoe added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 5, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@bcoe
Copy link
Contributor Author

bcoe commented Nov 6, 2020

@benjamingr happy with how this is looking?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

bcoe added a commit that referenced this pull request Nov 6, 2020
Using an iterable WeakMap (a data-structure that uses WeakRef and
WeakMap), we are able to: stop relying on Module._cache to
serialize source maps; stop requiring an error object when calling
findSourceMap().

PR-URL: #35915
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@bcoe
Copy link
Contributor Author

bcoe commented Nov 6, 2020

Landed in 8fa9035

@bcoe bcoe closed this Nov 6, 2020
@bcoe bcoe deleted the iterable-weak-map branch November 6, 2020 20:48
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
Using an iterable WeakMap (a data-structure that uses WeakRef and
WeakMap), we are able to: stop relying on Module._cache to
serialize source maps; stop requiring an error object when calling
findSourceMap().

PR-URL: #35915
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
bcoe added a commit to bcoe/node-1 that referenced this pull request Mar 16, 2021
Using an iterable WeakMap (a data-structure that uses WeakRef and
WeakMap), we are able to: stop relying on Module._cache to
serialize source maps; stop requiring an error object when calling
findSourceMap().

PR-URL: nodejs#35915
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
bcoe added a commit to bcoe/node-1 that referenced this pull request Mar 23, 2021
Using an iterable WeakMap (a data-structure that uses WeakRef and
WeakMap), we are able to: stop relying on Module._cache to
serialize source maps; stop requiring an error object when calling
findSourceMap().

PR-URL: nodejs#35915
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request May 16, 2021
Using an iterable WeakMap (a data-structure that uses WeakRef and
WeakMap), we are able to: stop relying on Module._cache to
serialize source maps; stop requiring an error object when calling
findSourceMap().

PR-URL: #35915
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants