Events: Separate the creation of json object for event from logging it #1360
Conversation
|
@BonelessImpl can you fix tests? |
Will do. The tests depend on going from json to strings directly, but because |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1360 +/- ##
==========================================
+ Coverage 80.29% 80.31% +0.02%
==========================================
Files 104 104
Lines 15243 15264 +21
==========================================
+ Hits 12239 12260 +21
Misses 3004 3004 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Tests work now. |
|
Btw, I recommend enabling the preserve_order feature in serde_json, since this is a blockchain and order matters. My tests make order not matter anymore. |
There was a problem hiding this comment.
@race-of-sloths score 3
@BonelessImpl Thank you for the contribution!
|
@frol Thank you for calling! @BonelessImpl Thank you for the contribution! Join Race of Sloths by simply mentioning me in your comment/PRs description and start collecting Sloth Points through contributions to open source projects. What is the Race of SlothsRace of Sloths is a friendly competition where you can participate in challenges and compete with other open-source contributors within your normal workflow For contributors:
For maintainers:
Feel free to check our website for additional details! Bot commands
|
near-sdk-macros/src/lib.rs
Outdated
| ::near_sdk::serde_json::to_value(event) | ||
| .unwrap_or_else(|_| ::near_sdk::env::abort()) | ||
| } | ||
|
|
||
| pub fn emit(&self) { | ||
| let json = self.to_json(); | ||
| let json_str = ::near_sdk::serde_json::to_string(&json) |
There was a problem hiding this comment.
Well, on the second look I realized that this adds unnecessary double serialization - first to Value and then to String. This means that we will be able to generate 2x fewer events in a single call. It was a critical requirement to generate as many events in 300TGas as possible.
Please, refactor it so there is no unnecessary performance penalty. Bonus points for having a benchmark test for it.
There was a problem hiding this comment.
Well, on the second look I realized that this adds unnecessary double serialization - first to Value and then to String. This means that we will be able to generate 2x fewer events in a single call. It was a critical requirement to generate as many events in 300TGas as possible.
Please, refactor it so there is no unnecessary performance penalty. Bonus points for having a benchmark test for it.
You're correct that it serializes twice. The problem there is that the EventBuilder type, which builds the json, is an opaque type that can't be taken outside the function that creates it. I will see what Rust wizardry I can do to make it happen only once.
There was a problem hiding this comment.
Unfortunately, there is no solution that will provide a zero cost abstraction. Especially one that doesn't do an extra heap allocation. So, a simpler solution is to just copy the implementation from emit(), and add tests that ensure that both emit the same result, whether json or not.
|
PR is ready for another review. Thank you guys! |
There was a problem hiding this comment.
@dj8yfo I also don't know a better solution from the top of my head. I don't like the duplication, but I won't block this PR for this reason - I hope dead code elimination in Rust compiler is good enough to remove the unused code.
|
@race-of-sloths score 5 |
|
@frol Thank you for calling! @BonelessImpl Thank you for the contribution! Join Race of Sloths by simply mentioning me in your comment/PRs description and start collecting Sloth Points through contributions to open source projects. What is the Race of SlothsRace of Sloths is a friendly competition where you can participate in challenges and compete with other open-source contributors within your normal workflow For contributors:
For maintainers:
Feel free to check our website for additional details! Bot commands
|
For simulation and testing purposes, there is currently no way to get the would-be logged DIP-4 event.
This PR does not change the current functionality of events, as
emit()does what it does before, but it extracts the creation of the json event into a functionto_json().