Performance of Object.defineProperties use in the CID class #200
Labels
Comments
|
That's a good idea. It could be paired with adding getters for the readonly properties. |
|
I'm not so tied to them needing to be read-only or hidden. Private class fields are only going to help in 3 of the cases, but even there we still kind of want access to them externally— getters are another possibility we could consider. Could you rerun your profiling with those fields as getters instead and see if it makes a difference? /cc @Gozala who will probably have opinions too—although I suspect that his typescript perspective might make him lean toward saying the annotations are good enough protection? |
achingbrain
added a commit
that referenced
this issue
Sep 22, 2022
`Object.defineProperties` is a performance bottleneck in applications that create lots and lots of CIDs (e.g. IPFS) so this PR removes it. The `asCID` property is changed to be a [private class field](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields) which requires increasing the minimum supported EcmaScript version but I don't know if that's a big deal or not. It does seem to make the property non-enumerable though. The CID class now implements a `Link` interface that has public `byteOffset` and `byteLength` properties so these become regular properties `code`, `version`, `multihash` and `bytes` become writable/configurable but they are marked with `@readonly` so maybe that's enough? Fixes #200
This was referenced Sep 22, 2022
Closed
I do indeed have no opinion in regards to this & would not mind removing whole |
rvagg
pushed a commit
that referenced
this issue
Oct 8, 2022
`Object.defineProperties` is a performance bottleneck in applications that create lots and lots of CIDs (e.g. IPFS) so this PR removes it. The `asCID` property is changed to be a [private class field](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields) which requires increasing the minimum supported EcmaScript version but I don't know if that's a big deal or not. It does seem to make the property non-enumerable though. The CID class now implements a `Link` interface that has public `byteOffset` and `byteLength` properties so these become regular properties `code`, `version`, `multihash` and `bytes` become writable/configurable but they are marked with `@readonly` so maybe that's enough? Fixes #200
rvagg
pushed a commit
that referenced
this issue
Oct 8, 2022
`Object.defineProperties` is a performance bottleneck in applications that create lots and lots of CIDs (e.g. IPFS) so this PR removes it. The `asCID` property is changed to be a [private class field](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields) which requires increasing the minimum supported EcmaScript version but I don't know if that's a big deal or not. It does seem to make the property non-enumerable though. The CID class now implements a `Link` interface that has public `byteOffset` and `byteLength` properties so these become regular properties `code`, `version`, `multihash` and `bytes` become writable/configurable but they are marked with `@readonly` so maybe that's enough? Fixes #200
rvagg
pushed a commit
that referenced
this issue
Oct 12, 2022
`Object.defineProperties` is a performance bottleneck in applications that create lots and lots of CIDs (e.g. IPFS) so this PR removes it. The `asCID` property is changed to be a [private class field](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields) which requires increasing the minimum supported EcmaScript version but I don't know if that's a big deal or not. It does seem to make the property non-enumerable though. The CID class now implements a `Link` interface that has public `byteOffset` and `byteLength` properties so these become regular properties `code`, `version`, `multihash` and `bytes` become writable/configurable but they are marked with `@readonly` so maybe that's enough? Fixes #200
rvagg
pushed a commit
that referenced
this issue
Oct 12, 2022
`Object.defineProperties` is a performance bottleneck in applications that create lots and lots of CIDs (e.g. IPFS) so this PR removes it. The `asCID` property is changed to be a [private class field](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields) which requires increasing the minimum supported EcmaScript version but I don't know if that's a big deal or not. It does seem to make the property non-enumerable though. The CID class now implements a `Link` interface that has public `byteOffset` and `byteLength` properties so these become regular properties `code`, `version`, `multihash` and `bytes` become writable/configurable but they are marked with `@readonly` so maybe that's enough? Fixes #200
github-actions bot
pushed a commit
that referenced
this issue
Oct 12, 2022
## [10.0.0](v9.9.0...v10.0.0) (2022-10-12) ### ⚠ BREAKING CHANGES * remove use of Object.defineProperties in CID class * use aegir for ESM-only build/testing/release ### Features * add complete set of aegir-based scripts ([1190bc6](1190bc6)) * define Link interface for CID ([88e29ea](88e29ea)) * remove deprecated CID properties & methods ([ffc4e6f](ffc4e6f)) * use aegir for ESM-only build/testing/release ([163d463](163d463)) ### Bug Fixes * --no-cov for all but chrome main ([b92f25f](b92f25f)) * add "browser" field, remove named local imports ([d60ea06](d60ea06)) * additional lint items from Link interface work ([91f677b](91f677b)) * address JS & TS linting complaints ([c12db2a](c12db2a)) * changes for new lint rules ([e6c9957](e6c9957)) * distribute types in dist/types/ ([c6defdb](c6defdb)) * ensure "master" as release branch ([16f8d9e](16f8d9e)) * make CID#asCID a regular property ([a74f1c7](a74f1c7)) * only release on master ([d15f26f](d15f26f)) * properly export types, build more complete pack ([8172ea8](8172ea8)) * remove "main" ([ad3306c](ad3306c)) * remove use of Object.defineProperties in CID class ([6149fae](6149fae)), closes [#200](#200) * run coverage only where it's supposed to ([872d121](872d121)) * test on all branches and pull requests ([f2ae077](f2ae077)) * ts-use import path ([53651c1](53651c1)) * use extensions for relative ts imports ([451998a](451998a)), closes [/github.com//pull/199#issuecomment-1252793515](https://github.com/multiformats//github.com/multiformats/js-multiformats/pull/199/issues/issuecomment-1252793515) * use parent `tsc` in ts-use ([85a9296](85a9296)) ### Tests * check for non-enumerability of asCID property ([b4ba07d](b4ba07d)) ### Trivial Changes * add test for structural copying ([#206](#206)) ([e8def36](e8def36)) * **no-release:** bump @types/mocha from 9.1.1 to 10.0.0 ([#205](#205)) ([a9a9347](a9a9347)) * **no-release:** bump actions/setup-node from 3.4.1 to 3.5.0 ([#204](#204)) ([604ca1f](604ca1f))
|
🎉 This issue has been resolved in version 10.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I'm doing some profiling to debug high CPU usage and
Object.definePropertiestakes up rather a lot of execution time:There's an interesting post here where someone tries to optimise very similar usage, an interesting quote being:
Would it be so terrible to remove the Object.definedProperties invocation here?
If we absolutely must have some sort of no-you-cannot-access-them-don't-even-think-about-it-the-sky-will-fall-on-our-heads-type protection for these fields could we switch to using private class fields instead?
The text was updated successfully, but these errors were encountered: