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

Optimize IdentifierIssuer existing Map. #65

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@
- Node.js using the improved browser algorithm can be ~4-9% faster overall.
- Node.js native `Buffer` conversion can be ~5-12% faster overall.
- Optimize a N-Quads serialization call.
- Optimize the `IdentifierIssuer` `existing` `Map`:
- Change clones from always-copy to copy-on-write.
- Depending on shape of data, this can reduce `Map` copies by ~90%. However,
in some data patterns, these will have very few entries, so overall
performance may not be noticeably effected.

### Fixed
- Disable native lib tests in a browser.
Expand Down
41 changes: 31 additions & 10 deletions lib/IdentifierIssuer.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2016-2021 Digital Bazaar, Inc. All rights reserved.
* Copyright (c) 2016-2023 Digital Bazaar, Inc. All rights reserved.
*/
'use strict';

Expand All @@ -9,12 +9,14 @@ module.exports = class IdentifierIssuer {
* identifiers, keeping track of any previously issued identifiers.
*
* @param prefix the prefix to use ('<prefix><counter>').
* @param existing an existing Map to use.
* @param existing an existing refs and Map object to use.
* @param counter the counter to use.
*/
constructor(prefix, existing = new Map(), counter = 0) {
constructor(prefix, existing = {refs: 0, map: new Map()}, counter = 0) {
this.prefix = prefix;
this._existing = existing;
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the prefixed _ so only protected / internal usage of this is expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. But it's not consistent. counter is only used internally, but has no leading _.

this.existing = existing;
// add ref to shared map
this.existing.refs++;
this.counter = counter;
}

Expand All @@ -24,8 +26,8 @@ module.exports = class IdentifierIssuer {
* @return a copy of this IdentifierIssuer.
*/
clone() {
const {prefix, _existing, counter} = this;
return new IdentifierIssuer(prefix, new Map(_existing), counter);
const {prefix, existing, counter} = this;
return new IdentifierIssuer(prefix, existing, counter);
}

/**
Expand All @@ -38,7 +40,7 @@ module.exports = class IdentifierIssuer {
*/
getId(old) {
// return existing old identifier
const existing = old && this._existing.get(old);
const existing = old && this.existing.map.get(old);
if(existing) {
return existing;
}
Expand All @@ -49,7 +51,26 @@ module.exports = class IdentifierIssuer {

// save mapping
if(old) {
this._existing.set(old, identifier);
if(this.existing.refs > 1) {
// copy-on-write shared map
// TODO: improve copy-on-write reference handling
// - current code handles copying the 'existing' maps when it is
// shared
// - it will remove a reference when doing a copy
// - a reference is NOT removed when a copy is no longer used
// - need a `release()` call or similar to do this and add it
// throughout the code as needed
// - this won't result in errors, only extra copies if a child does
// not do an update, is done, and a parent then does an update
// unref shared map
this.existing.refs--;
// copy to new map
this.existing = {
refs: 1,
map: new Map(this.existing.map)
};
}
this.existing.map.set(old, identifier);
}

return identifier;
Expand All @@ -65,7 +86,7 @@ module.exports = class IdentifierIssuer {
* false if not.
*/
hasId(old) {
return this._existing.has(old);
return this.existing.map.has(old);
}

/**
Expand All @@ -75,6 +96,6 @@ module.exports = class IdentifierIssuer {
* @return the list of old IDs that has been issued new IDs in order.
*/
getOldIds() {
return [...this._existing.keys()];
return [...this.existing.map.keys()];
}
};