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

refactor: optimize memoize use, misc cases #10122

Merged
merged 5 commits into from Nov 18, 2018

Conversation

SimonSchick
Copy link
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Followup of #10091, re-implements the previous changes that were dropped for being too out of scope.

This PR also optimizes/streamlines type checks eg. _.isType to typeof ... === 'type', lodash is generally slower for those as it uses alternative methods for type checking that are compatible with legacy browsers and the likes.

@SimonSchick SimonSchick force-pushed the optimize/memoize branch 3 times, most recently from a055458 to 057a566 Compare November 5, 2018 02:58
@codecov
Copy link

codecov bot commented Nov 5, 2018

Codecov Report

Merging #10122 into master will decrease coverage by 0.01%.
The diff coverage is 95.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10122      +/-   ##
==========================================
- Coverage   96.36%   96.34%   -0.02%     
==========================================
  Files          63       63              
  Lines        9441     9419      -22     
==========================================
- Hits         9098     9075      -23     
- Misses        343      344       +1
Impacted Files Coverage Δ
lib/dialects/abstract/connection-manager.js 90% <100%> (ø) ⬆️
lib/model.js 96.77% <100%> (-0.05%) ⬇️
lib/dialects/mssql/query-generator.js 95.83% <100%> (ø) ⬆️
lib/dialects/mysql/query.js 98.46% <100%> (ø) ⬆️
lib/utils.js 98.26% <100%> (-0.02%) ⬇️
lib/transaction.js 100% <100%> (ø) ⬆️
lib/dialects/postgres/query-generator.js 94.08% <100%> (ø) ⬆️
lib/dialects/postgres/data-types.js 97.87% <100%> (ø) ⬆️
lib/instance-validator.js 97.54% <100%> (ø) ⬆️
lib/errors/index.js 100% <100%> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce9287f...ac3ae90. Read the comment docs.

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Choose a reason for hiding this comment

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

LGTM, just one change

lib/model.js Show resolved Hide resolved
lib/utils.js Outdated
* @private
*/
function addSetToArray(array, set) {
for (const v of set) {
Copy link
Contributor

Choose a reason for hiding this comment

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

return array.concat(Array.from(set))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'use strict';

function addSetToArray(array, set) {
  for (const v of set) {
    array.push(v);
  }
}

const set = new Set([1, 2, 3, 4, 5, 6, 7, 8, 9]);

console.time('addSetToArray');
for(let i = 0;i<0xffffff;++i) {
  addSetToArray([], set);
}
console.timeEnd('addSetToArray');

console.time('Array.from');
for(let i = 0;i<0xffffff;++i) {
  [].concat(Array.from(set));
}
console.timeEnd('Array.from');
addSetToArray: 736.759ms
Array.from: 20121.014ms

On my machine using .concat(Array.from(set)) is about 25x slower.

The intention here is to make the code faster using sets, converting sets to arrays is a rather expensive operation, addSetToArray is the fastest way to do it pretty much, using Array.from would nullify all performance gained 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally verified and Array.from is indeed really slow. Bummer, it was really concise. Will go with for + push approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimonSchick
Copy link
Contributor Author

SimonSchick commented Nov 12, 2018

Uhh, wait, why did you merge master into my branch?

I guess I enabled you to edit and there were conflicts, I would've rebased it for you :)

@SimonSchick
Copy link
Contributor Author

@sushantdhiman mergy merge?

lib/model.js Outdated
if (this.constructor._hasVirtualAttributes) {
keys = keys.concat(this.constructor._virtualAttributes);
Utils.addSetToArray(keys, this.constructor._virtualAttributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't merged because this still doesn't looks right to me, I think we should keep using array for _virtualAttributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason why?
The overall code will be faster and it's not any less readable :P

Copy link
Contributor

Choose a reason for hiding this comment

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

We are explicitly converting set to an array, this points out what we really need here is an array. Also simple concat on array is more readable and less complex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about the 5 other .has (previously .includes) calls? 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

has and includes got strong interchangeability , on other had for _virtualAttributes case we had to write special code to convert set to array, which indicate need of array rather than set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of this PR was to primarily to get rid of memoize + includes and replace with sets since that's slows all reads. Also keep in mind the set is only converted for iteration.

I could change the code in this location to not create arrays at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I could change the code in this location to not create arrays at all.

Hmm, give it a shot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, waiting for build, I'm in the process of moving atm so I might take a while to reply.

@sushantdhiman sushantdhiman merged commit 6b1ff3b into sequelize:master Nov 18, 2018
@sushantdhiman
Copy link
Contributor

Thanks @SimonSchick 👍

@SimonSchick SimonSchick deleted the optimize/memoize branch November 18, 2018 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants