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
feat(core): allow JsonType to use a stable JSON.stringify algorithm #3584
base: master
Are you sure you want to change the base?
feat(core): allow JsonType to use a stable JSON.stringify algorithm #3584
Conversation
This pull request introduces 1 alert when merging 3341e63 into 9d5d457 - view on LGTM.com new alerts:
|
Can you prepare some benchmarks so we know how much slower this solution is? If its fast enough, I dont mind doing this by default, but it would have to be very close to the native implementation. I am not sure if the type constructor is the right place for this (I do understand this is the "least modified code path"), a global config option would be definitely a good idea, and a property level option to have fine grained control. Because in generall you dont need to work with the type instance, a Maybe something like this? @Property({ type: 'json', stableStringify: true })
object: { foo: string; bar: number }; That way we could easily control it globally too, I guess if the user wants it stable, they will want to have it stable for all the JSON properties. I also dont mind having the constructor option in |
Codecov ReportBase: 99.84% // Head: 99.62% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3584 +/- ##
==========================================
- Coverage 99.84% 99.62% -0.22%
==========================================
Files 211 212 +1
Lines 13174 13207 +33
Branches 3053 3062 +9
==========================================
+ Hits 13153 13158 +5
- Misses 21 44 +23
- Partials 0 5 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Will do
yeah, makes sense 👍 to add some thoughs about json fields before doing more:
|
@B4nan the perf impact is massive for my use-case, if used everywhere ! 🔴 this should not be merged as is 😨 👉 One big problem is that registering managed entities seems to also serialize JSON values pre-emptively 😨 also, in my random current use case, >90%of json fields of my loaded models would be perfectly fine if compared as references, which I don't know how to do with current type abstraction. (side note: all those little perf problems really seem easilly fixable. I have a ~150seconds script, where most of the time is just MikroORM doing things => early profiling show quick win everywhere, despite the code already beeing very clean; I know you probably have many other things to do, but I'd really like to help optimize mikro-orm further ! Your code is really clean ! :) |
@B4nan your precisions have been noted. I'll need to take a couple days to dive deeper in the Mikro ORM codebase. In any case, @rvion profiling proves that more work is required. I'll get back to you. |
Maybe for your use case, the go to way shouldn't be replacing references, but object mutation instead and there you can't just compare references.
Yes, that's how it works, we need to compute the state that is later used for changeset computation. And due to possible differences in the input (based on differences when using the joined loading strategy mostly) we need to serialize the entity instead of using the data that came from database. I think we should add the compare method to the Indeed the alternative to this could be smarter diffing, so instead of handing custom serialization, we would just make sure the values are same regardless of the order of keys in objects. And if we have this new compare method, we could use it to compare JSON values via |
yeah, especially since it's impossible to hack around field order, since postgres will not preserve ordering on update like node does. But since node preserves the field oder from postgres, I went with your proposal to allow custom way to compare, without having to serialize first the fields would indeed solve all my problems. for this specific entity, I'll go with a custom And for for some of my other entities (with hundreds of thouthands of instances loaded in the EM in this very specific example), some of them have json fields known to be frozen and only assignable by reference, I'll go with an even simpler reference comparaison, thus saving seconds of runtime |
fixes #3583
We introduced an option on JsonType to allow user to choose between speed or better serialization.
I'm not sure if the option is required, or if stable stringify should simply be the only option because it's more robust ?
I'm not sure either of what you prefer as a default: slightly more speed or less diffs. In doubt, we kept the the old behaviour by default.
Feel free to decide what option is best.