Skip to content

Commit

Permalink
Optimize IdentifierIssuer existing Map.
Browse files Browse the repository at this point in the history
- 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.
  • Loading branch information
davidlehn committed Mar 31, 2023
1 parent 96d9570 commit 80e1b96
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 10 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@
- Improvement depends on number of digests performed.
- 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 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;
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()];
}
};

0 comments on commit 80e1b96

Please sign in to comment.