feat: improve performance and type-safety #83
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Replace maps with structs to achieve significant performance gains. (Benchmarks show that this implementation is now faster than jsonld.js.) - Improve type-safety. Previously, type confusion resulted in unnecessary checks, such as comparing a known string value against a boolean. - Ensure defaultLanguage always contains a language. Previously, a missing if-check could result in defaultLanguage being set to "_%s", with %s being the direction. - Remove GetKeysString since it's not used anywhere and is equivalent to the generic GetKeys now. Signed-off-by: Michael Adler <[email protected]>
|
@michaeladler, the idea behind this PR makes sense. I can see why it improves performance. The changes are substantial, and I'm still going through it line by line to ensure it does the right things. Apologies for the delay. I should finish the review this week. |
|
No problem, I completely understand. Thank you for your thorough review. The test suite is excellent though: it caught every mistake I made (such as using |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This may seem like a mundane merge request, but it delivers significant performance improvements. Benchmarks show that this library now outperforms jsonld.js.
Note: While the changes may appear straightforward, caution is required when implementing them. It is sometimes necessary to differentiate between a value explicitly set to
nullin the context and one that is simply absent.PS: I don't have any other changes lined up (I couldn't submit this PR earlier because it builds upon #82 to avoid merge conflicts).
Details:
Basic example
No change in behavior.
Motivation
Performance! Additionally, the struct-based design enhances code navigation/reasoning (especially when using an LSP).
Checks
make test