-
Notifications
You must be signed in to change notification settings - Fork 85
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
128 bit numbers #88
128 bit numbers #88
Conversation
Initial perf (just memory layout updates & functions, no parsing). Impact is measurable but not terrible, nothing to enable by default but as a flag for those who need it it's acceptable performance I think. 0.2.2 (master)
this
|
@mstump do you want to give this branch a try and see if it solves your issue? |
(marking this as non draft to get codecov to post it's stuff) - not ready to be merged yet but input is more then welcome |
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
==========================================
+ Coverage 84.65% 85.73% +1.07%
==========================================
Files 36 39 +3
Lines 4385 5284 +899
==========================================
+ Hits 3712 4530 +818
- Misses 673 754 +81
Continue to review full report at Codecov.
|
@sunnygleason this should be ready for review :) |
@sunnygleason could I bug you for a review so we can merge this? |
I finally climbed out from under my pile of demos and I'm sorry to report that I'm still getting weird behavior with this branch. Cargo snipit:
With the 128bit feature enabled I get the error message:
Where previously I got:
The enum that it's decoding is
The offending JSON is below, it cleanly deserializes with
|
That looks like I did something wrong in the serde interop, I’ll check tomorrow if I can work out what serde expects of us for 128 but support thanks for sharing the structure and data that go right into a test
Cheers,
Heinz
…________________________________
From: Matt Stump <[email protected]>
Sent: Monday, January 13, 2020 10:45:23 PM
To: simd-lite/simdjson-rs <[email protected]>
Cc: Heinz Gies <[email protected]>; Author <[email protected]>
Subject: Re: [simd-lite/simdjson-rs] 128 bit numbers (#88)
I finally climbed out from under my pile of demos and I'm sorry to report that I'm still getting weird behavior with this branch.
Cargo snipit:
[dependencies.simd-json]
git = "https://github.com/simd-lite/simdjson-rs"
branch = "128-bit-numbers"
features = ["128bit"]
With the 128bit feature enabled I get the error message:
thread 'reporter_message::raw_message::tests::test_deserialize_elastic_search' panicked at 'JsonSimdError(Error { index: 0, character: '💩', error: Serde("invalid type: i128, expected any value") })', src/reporter_message/raw_message.rs:398:25
Where previously I got:
thread 'reporter_message::raw_message::tests::test_deserialize_elastic_search' panicked at 'JsonSimdError(Error { index: 191048, character: '\n', error: Overflow })', src/reporter_message/raw_message.rs:398:25
The enum that it's decoding is
#[serde(untagged)]
#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
pub enum RawMessageMetricValue {
Boolean(bool),
Float(f64),
Long(i128),
Integer(i64),
Metric(Vec<RawMessageMetricNode>),
String(String),
}
The offending JSON is below, it cleanly deserializes with serde_json.
{
"name": "max_unsafe_auto_id_timestamp",
"value": -9223372036854776000
}
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#88>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAA5CNJTFOPL7VXWG3MVTKDQ5TOHHANCNFSM4JVOLEHQ>.
|
Turns out this is a bug in serde, it doesn't support untagged fields with 128 bit values :/ |
@Licenser what should the next step for this PR be? |
I opened a ticket with serde: serde-rs/serde#1717 I'm honestly not 100% sure, merging it would already allow 128 bit values outside of enums and should work there once it is fixed in serde. Since no code changes will be required I think we could still merge it, there is little reason to hold it back just because of a bug in serde. other then that clippy is throwing spanners in the works :P only 177 new errors! yay |
@sunnygleason can I bug you for a review so we can merge this? The remaining issue is out of our control and it will work once serde allows 128bit integers in untagged enums. So nothing we can do about that, our code is correct. |
@Licenser sure! Were you able to run fuzz testing for the feature? Just curious... |
I haven't it has been broken for a bit, and it feels with #95 in the todo list it's not worth investing time into unbreak it. We'll scrap the current fuzz stuff anyway. |
See #59
todo: