Skip to content

allow caching in sortable set and use it#5917

Merged
sokra merged 4 commits intonextfrom
refactor/cache-sortable-set
Nov 7, 2017
Merged

allow caching in sortable set and use it#5917
sokra merged 4 commits intonextfrom
refactor/cache-sortable-set

Conversation

@sokra
Copy link
Copy Markdown
Member

@sokra sokra commented Nov 3, 2017

What kind of change does this PR introduce?
refactor caching

Did you add tests for your changes?
existing tests

If relevant, link to documentation update:
N/A

Summary
more caching -> performance

Does this PR introduce a breaking change?
yes

Other information

Comment thread lib/Chunk.js
return str;
};

const getArray = set => Array.from(set);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

move to lib/util/toArray.js && getArray =>toArray ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think a separate module make sense for a single line of code. This increases code size, decreases performance and readablility.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, it isn't really needed here, especially after the last refactor :)

Comment thread lib/Chunk.js Outdated
return 0;
};

const getFrozenArray = set => Object.freeze(Array.from(this));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

move to lib/util/toFrozenArray.js && getFrozenArray =>toFrozenArray ?

Comment thread lib/Chunk.js Outdated
return 0;
};

const getFrozenArray = set => Object.freeze(Array.from(this));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

move to lib/util/toFrozenArray.js && getFrozenArray => toFrozenArray ?

Comment thread lib/Chunk.js
return str;
};

const getArray = set => Array.from(set);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

move to lib/util/toArray.js && getArray => toArray ?

Comment thread lib/Chunk.js Outdated
*/
getChunks() {
return Array.from(this._chunks);
return this._chunks.getCachedInfo(getArray);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

getCache(d) ?

Comment thread lib/Module.js Outdated
return a.debugId - b.debugId;
};

const getFrozenArray = set => Object.freeze(Array.from(this));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use lib/util/toFrozenArray.js ? :)

Comment thread lib/Chunk.js Outdated
str += m.identifier() + "#";
});
return str;
return this._modules.getOrderIndependentCachedInfo(getModulesIdent);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

getOrderIndependentCache(d)

OrderIndenpendent => Unordered/Unsorted/Orderless

@webpack-bot
Copy link
Copy Markdown
Contributor

@sokra Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@michael-ciniawsky Please review the new changes.

@webpack-bot
Copy link
Copy Markdown
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@sokra sokra merged commit 49cdb94 into next Nov 7, 2017
@sokra sokra deleted the refactor/cache-sortable-set branch November 7, 2017 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants