Skip to content

Commit

Permalink
Improve File hashing logic in bundler.js.
Browse files Browse the repository at this point in the history
Follow-up to #9933.

As recommended by @abernix, the sha1 hash of every file is now computed
from the file's sha512 hash, so we don't have to hash the entire contents
of the file twice with two different algorithms.

Other changes/improvements:

* Invalidate the hashes when/if `File#setContents` is called.

* Ignore `options.hash` and just compute hashes from actual file contents.
  Disagreement here would be worse than any performance benefits from
  precomputing the hash.
  • Loading branch information
benjamn committed Jul 3, 2018
1 parent ef3af68 commit 6246e41
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 20 deletions.
10 changes: 5 additions & 5 deletions tools/fs/watch.js
Expand Up @@ -261,19 +261,19 @@ export function readFile(absPath) {
};

export function sha1(...args) {
return Profile("sha1", function () {
return Profile.run("sha1", function () {
var hash = createHash('sha1');
args.forEach(arg => hash.update(arg));
return hash.digest('hex');
})();
});
}

export function sri(...args) {
return Profile("sri", function () {
export function sha512(...args) {
return Profile.run("sha512", function () {
var hash = createHash('sha512');
args.forEach(arg => hash.update(arg));
return hash.digest('base64');
})();
});
}

export function readDirectory({absPath, include, exclude, names}) {
Expand Down
22 changes: 7 additions & 15 deletions tools/isobuild/bundler.js
Expand Up @@ -529,8 +529,6 @@ export class NodeModulesDirectory {
// Allowed options:
// - sourcePath: path to file on disk that will provide our contents
// - data: contents of the file as a Buffer
// - hash: optional, sha1 hash of the file contents, if known
// - sri: sha512 hash of file contents in base64 encoding
// - sourceMap: if 'data' is given, can be given instead of
// sourcePath. a string or a JS Object. Will be stored as Object.
// - cacheable
Expand Down Expand Up @@ -599,7 +597,6 @@ class File {
this.assets = null;

this._contents = options.data || null; // contents, if known, as a Buffer
this._hashOfContents = options.hash || null;
this._hash = null;
this._sri = null;
}
Expand All @@ -615,13 +612,9 @@ class File {

hash() {
if (! this._hash) {
if (! this._hashOfContents) {
this._hashOfContents = watch.sha1(this.contents());
}

this._hash = watch.sha1(
String(File._salt()),
this._hashOfContents,
this.sri(),
);
}

Expand All @@ -630,10 +623,11 @@ class File {

sri() {
if (! this._sri) {
this._sri = watch.sri(this.contents());
this._sri = watch.sha512(this.contents());
}

return this._sri;
}
}

// Omit encoding to get a buffer, or provide something like 'utf8'
// to get a string
Expand All @@ -650,12 +644,10 @@ class File {
}

setContents(b) {
if (!(b instanceof Buffer)) {
throw new Error("Must set contents to a Buffer");
}
assert.ok(Buffer.isBuffer(b), "Must pass Buffer to File#setContents");
this._contents = b;
// Un-cache hash.
this._hashOfContents = this._hash = null;
// Bust the hash cache.
this._hash = this._sri = null;
}

size() {
Expand Down

0 comments on commit 6246e41

Please sign in to comment.