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

protocol/packets/Field.js typeToString(t) uses O(n) lookup #2170

Closed
mbecker16 opened this issue Jan 23, 2019 · 5 comments
Closed

protocol/packets/Field.js typeToString(t) uses O(n) lookup #2170

mbecker16 opened this issue Jan 23, 2019 · 5 comments
Assignees

Comments

@mbecker16
Copy link

@mbecker16 mbecker16 commented Jan 23, 2019

I was doing some performance profiling with sequelize and found that a lot of time was being spent in the typeToString(t) function in protocol/packets/Field.js
It's doing a linear lookup in the protocol/contants/types.js file. I switched this to a create a lookup table when the module is loaded and noticed a considerable speed up.

I'm happy to make the code change to this file but wanted to know if there anyone has a preferred approached.

Do you prefer to have lookup table (map) created in the protocol/packets/Field.js file when the module is loaded?
or
Have the lookup by gex put in the protocol/constants/types.js file as it is in the protocol/constants/errors.js file?

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jan 23, 2019

(1)

@sidorares
Copy link
Member

@sidorares sidorares commented Jan 23, 2019

Yes, I think doing similar to errors.js ( thought shorter array with just lookup by code and no holes might be much faster)

@mbecker16
Copy link
Author

@mbecker16 mbecker16 commented Jan 23, 2019

Would it be reasonable to put them in seperate file named TypesLookup.js and use that file in Field.js?

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jan 23, 2019

Just dynamically create lookup table from Types export inside Field.js and save in variable.

@mbecker16
Copy link
Author

@mbecker16 mbecker16 commented Jan 23, 2019

Ok, thanks.

@dougwilson dougwilson self-assigned this Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants