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.

Sign up for GitHub

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

Jump to bottom

Sortable set #5083

Merged
merged 10 commits into from
Jun 21, 2017
Merged

Sortable set #5083

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0eaa847
add SortableSet as a new collection type
timse Jun 18, 2017
747efca
use SortableSet in chunks
timse Jun 18, 2017
4e5ef0d
use SortableSet in modules
timse Jun 18, 2017
bd8c6cf
make sure ConcatenatedModules also uses SortedSet
timse Jun 18, 2017
1a16a3e
make sort function private
timse Jun 19, 2017
7a99930
optimize sortableSet to prevent unnecessary sorting
timse Jun 19, 2017
f7ffed8
allow to set chunks and use it in ConcatenatedModule
timse Jun 20, 2017
5f16dd8
clean up SortableSet
timse Jun 20, 2017
b2a4244
add tests for sortableset
timse Jun 20, 2017
8f3d65a
fix linting/formatting issues
timse Jun 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 21 additions & 27 deletions lib/Chunk.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,29 @@

const util = require("util");
const compareLocations = require("./compareLocations");
const SortableSet = require("./util/SortableSet");
let debugId = 1000;

const byId = (a, b) => {
if(a.id < b.id) return -1;
if(b.id < a.id) return 1;
return 0;
};

class Chunk {

static sortById(a, b) {
Copy link
Member

Choose a reason for hiding this comment

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

Why making this methods public? What was wrong with the original way, which hides them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially I wanted to flip them, so sortByXXX was in Module and Chunk would use Module.sortByXXX to sort its Modules.
As Module should know how to sort modules, not Chunk.
However node and circular dependencies let me down and so this is a left over of that approach, happy to make them private again, but I'd rather find a solution for the intial idea. :D

if(a.id < b.id) return -1;
if(b.id < a.id) return 1;
return 0;
}

static sortByIdentifier(a, b) {
if(a.identifier() > b.identifier()) return 1;
if(a.identifier() < b.identifier()) return -1;
return 0;
}

constructor(name, module, loc) {
this.id = null;
this.ids = null;
this.debugId = debugId++;
this.name = name;
this._modules = new Set();
this._modulesIsSorted = true;
this._modules = new SortableSet(undefined, Chunk.sortByIdentifier);
this.entrypoints = [];
this.chunks = [];
this.parents = [];
Expand Down Expand Up @@ -92,7 +98,6 @@ class Chunk {
addModule(module) {
if(!this._modules.has(module)) {
this._modules.add(module);
this._modulesIsSorted = false;
return true;
}
return false;
Expand Down Expand Up @@ -139,8 +144,7 @@ class Chunk {
}

setModules(modules) {
this._modules = new Set(modules);
this._modulesIsSorted = false;
this._modules = new SortableSet(modules, Chunk.sortByIdentifier);
}

getNumberOfModules() {
Expand All @@ -159,19 +163,9 @@ class Chunk {
return Array.from(this._modules, fn);
}

_ensureModulesSorted() {
if(this._modulesIsSorted) return;
this._modules = new Set(Array.from(this._modules).sort((a, b) => {
if(a.identifier() > b.identifier()) return 1;
if(a.identifier() < b.identifier()) return -1;
return 0;
}));
this._modulesIsSorted = true;
}

compareTo(otherChunk) {
this._ensureModulesSorted();
otherChunk._ensureModulesSorted();
this._modules.sort();
otherChunk._modules.sort();
if(this._modules.size > otherChunk._modules.size) return -1;
if(this._modules.size < otherChunk._modules.size) return 1;
const a = this._modules[Symbol.iterator]();
Expand All @@ -196,7 +190,7 @@ class Chunk {
}

getModulesIdent() {
this._ensureModulesSorted();
this._modules.sort();
let str = "";
this._modules.forEach(m => {
str += m.identifier() + "#";
Expand Down Expand Up @@ -428,7 +422,7 @@ class Chunk {
}

sortItems() {
this._modules = new Set(Array.from(this._modules).sort(byId));
this._modules.sortWith(Chunk.sortById);
this.origins.sort((a, b) => {
const aIdent = a.module.identifier();
const bIdent = b.module.identifier();
Expand All @@ -440,8 +434,8 @@ class Chunk {
if(origin.reasons)
origin.reasons.sort();
});
this.parents.sort(byId);
this.chunks.sort(byId);
this.parents.sort(Chunk.sortById);
this.chunks.sort(Chunk.sortById);
}

toString() {
Expand Down
50 changes: 17 additions & 33 deletions lib/Module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,24 @@
"use strict";

const util = require("util");

const DependenciesBlock = require("./DependenciesBlock");
const ModuleReason = require("./ModuleReason");
const SortableSet = require("./util/SortableSet");
const Template = require("./Template");

function byId(a, b) {
return a.id - b.id;
}

function byDebugId(a, b) {
return a.debugId - b.debugId;
}

let debugId = 1000;

class Module extends DependenciesBlock {

static sortById(a, b) {
return a.id - b.id;
}

static sortByDebugId(a, b) {
return a.debugId - b.debugId;
}

constructor() {
super();
this.context = null;
Expand All @@ -34,9 +37,7 @@ class Module extends DependenciesBlock {
this.used = null;
this.usedExports = null;
this.providedExports = null;
this._chunks = new Set();
this._chunksIsSorted = true;
this._chunksIsSortedByDebugId = true;
this._chunks = new SortableSet(undefined, Module.sortById);
this._chunksDebugIdent = undefined;
this.warnings = [];
this.dependenciesWarnings = [];
Expand All @@ -59,7 +60,6 @@ class Module extends DependenciesBlock {
this.providedExports = null;
this._chunks.clear();
this._chunksDebugIdent = undefined;
this._chunksIsSorted = this._chunksIsSortedByDebugId = false;
super.disconnect();
}

Expand All @@ -71,14 +71,12 @@ class Module extends DependenciesBlock {
this.depth = null;
this._chunks.clear();
this._chunksDebugIdent = undefined;
this._chunksIsSorted = this._chunksIsSortedByDebugId = false;
super.unseal();
}

addChunk(chunk) {
this._chunks.add(chunk);
this._chunksDebugIdent = undefined;
this._chunksIsSorted = this._chunksIsSortedByDebugId = false;
}

removeChunk(chunk) {
Expand All @@ -96,7 +94,7 @@ class Module extends DependenciesBlock {

getChunkIdsIdent() {
if(this._chunksDebugIdent !== undefined) return this._chunksDebugIdent;
this._ensureChunksSortedByDebugId();
this._chunks.sortWith(Module.sortByDebugId);
const chunks = this._chunks;
const list = [];
for(const chunk of chunks) {
Expand Down Expand Up @@ -130,8 +128,8 @@ class Module extends DependenciesBlock {

hasEqualsChunks(otherModule) {
if(this._chunks.size !== otherModule._chunks.size) return false;
this._ensureChunksSortedByDebugId();
otherModule._ensureChunksSortedByDebugId();
this._chunks.sortWith(Module.sortByDebugId);
otherModule._chunks.sortWith(Module.sortByDebugId);
const a = this._chunks[Symbol.iterator]();
const b = otherModule._chunks[Symbol.iterator]();
while(true) { // eslint-disable-line
Expand All @@ -142,20 +140,6 @@ class Module extends DependenciesBlock {
}
}

_ensureChunksSorted() {
if(this._chunksIsSorted) return;
this._chunks = new Set(Array.from(this._chunks).sort(byId));
this._chunksIsSortedByDebugId = false;
this._chunksIsSorted = true;
}

_ensureChunksSortedByDebugId() {
if(this._chunksIsSortedByDebugId) return;
this._chunks = new Set(Array.from(this._chunks).sort(byDebugId));
this._chunksIsSorted = false;
this._chunksIsSortedByDebugId = true;
}

addReason(module, dependency) {
this.reasons.push(new ModuleReason(module, dependency));
}
Expand Down Expand Up @@ -221,8 +205,8 @@ class Module extends DependenciesBlock {
sortItems(sortChunks) {
super.sortItems();
if(sortChunks)
this._ensureChunksSorted();
this.reasons.sort((a, b) => byId(a.module, b.module));
this._chunks.sort();
this.reasons.sort((a, b) => Module.sortById(a.module, b.module));
if(Array.isArray(this.usedExports)) {
this.usedExports.sort();
}
Expand Down
3 changes: 2 additions & 1 deletion lib/optimize/ConcatenatedModule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
const Module = require("../Module");
const Template = require("../Template");
const Parser = require("../Parser");
const SortedSet = require("../util/SortableSet");
const acorn = require("acorn");
const escope = require("escope");
const ReplaceSource = require("webpack-sources/lib/ReplaceSource");
Expand Down Expand Up @@ -135,7 +136,7 @@ class ConcatenatedModule extends Module {
this.providedExports = rootModule.providedExports;
this.optimizationBailout = rootModule.optimizationBailout;
this.used = rootModule.used;
this._chunks = new Set(rootModule._chunks);
this._chunks = new SortedSet(rootModule._chunks, Module.sortById);
this.index = rootModule.index;
this.index2 = rootModule.index2;
this.depth = rootModule.depth;
Expand Down
28 changes: 28 additions & 0 deletions lib/util/SortableSet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
"use strict";

module.exports = class SortableSet extends Set {

constructor(initialIterable, defaultSort) {
super(initialIterable);
this._sortFn = defaultSort;
}

/**
* @param {Function} sortFn - function to sort the set
* @returns {void}
*/
sortWith(sortFn) {
const sortedArray = Array.from(this).sort(sortFn);
Copy link
Member

Choose a reason for hiding this comment

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

The original impl checked if the set was already sorted before sorting. The new SortableSet should do the same. add and clear should be overriden to invalidate sorting. Best store the current active sortFn.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I wasn't sure if this approach is ok and if I wanted to shadow as little as possible from the initial set (: happy to add some logic there

this.clear();
for(let i = 0; i < sortedArray.length; i += 1) {
this.add(sortedArray[i]);
}
}

/**
* @returns {void}
*/
sort() {
this.sortWith(this._sortFn);
}
};
2 changes: 1 addition & 1 deletion test/statsCases/scope-hoisting-multi/expected.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ Child
[0] (webpack)/test/statsCases/scope-hoisting-multi/common_lazy_shared.js 25 bytes {0} {1} {2} [built]
[1] (webpack)/test/statsCases/scope-hoisting-multi/vendor.js 25 bytes {5} [built]
ModuleConcatenation (inner): module is an entrypoint
[2] (webpack)/test/statsCases/scope-hoisting-multi/common.js + 1 modules 62 bytes {4} {3} [built]
[2] (webpack)/test/statsCases/scope-hoisting-multi/common.js + 1 modules 62 bytes {3} {4} [built]
Copy link
Member

Choose a reason for hiding this comment

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

This should not happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

see: bd8c6cf

fix tests that was falsy - per default chunks are flagged as sorted in module
however this only holds true as they are initialized empty. Concatenated module however
has initial modules and therefore is not guaranteed to be ordered, the flags should therfor be false.
Using SortedSet fixes this as a sideeffect

Copy link
Member

Choose a reason for hiding this comment

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

Ah okey. 👍

Copy link
Member

Choose a reason for hiding this comment

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

It seem to be different again...

Copy link
Member Author

Choose a reason for hiding this comment

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

ha yea, it tries to use the sort method from Module but its no longer exposed...

meh, i'd love to just have a compareBy method on them :P maybe in another PR if thats ok :D

[3] (webpack)/test/statsCases/scope-hoisting-multi/common_lazy.js 25 bytes {1} {2} [built]
[4] (webpack)/test/statsCases/scope-hoisting-multi/first.js + 1 modules 238 bytes {4} [built]
ModuleConcatenation (inner): module is an entrypoint
Expand Down