Skip to content

Index with [] and some other API uses#36

Closed
masklinn wants to merge 2 commits intoTritonDataCenter:rustfrom
masklinn:api
Closed

Index with [] and some other API uses#36
masklinn wants to merge 2 commits intoTritonDataCenter:rustfrom
masklinn:api

Conversation

@masklinn
Copy link

@masklinn masklinn commented Aug 31, 2018

  • replaced a bunch of .get().unwrap() by [], they may have been used for a good reason though, IDK
  • I feel using getopt's helpers (optflag and optopt) make Opt completely unnecessary
  • use various stdlib API to — I feel — implify some snippets

More of the JSON stuff can also be shoved onto serde via StreamDeserializer.

println!("{},", datum[i]);
}

println!("{}", datum[datum.len() - 1]);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have misunderstood this bit entirely, but I really don't understand why it was not in the loop. It doesn't seem to be any different.

@masklinn masklinn mentioned this pull request Sep 1, 2018
@dtolnay
Copy link
Contributor

dtolnay commented Sep 4, 2018

This PR regresses parse time by more than 50% -- from 10.1 seconds to 15.7 seconds as measured by my setup in #40. That seems likely to be from the serde commit. I haven't looked closely at the other two commits here but consider removing the serde one or moving it to a different PR.

@masklinn
Copy link
Author

masklinn commented Sep 4, 2018

Would make sense, I'll check on your setup to verify though, might be one of the other bits.

Why do you figure that would be though, the dispatching between the cases of the untagged enums within serde? I see that in #40 you're using StreamDeserializer but keeping the record-at-a-time logic of the original instead of handing that out to serde.

@dtolnay
Copy link
Contributor

dtolnay commented Sep 4, 2018

Serde's approach for untagged enums is not yet optimized that well. The input data happens to be almost 90% that first variant, so my implementation goes directly to the correct variant almost every time.

@masklinn
Copy link
Author

masklinn commented Sep 4, 2018

Makes sense. I'd have assumed serde to do the same thing since I kept the original order in the enum. I guess I'll remove the serde commit since it's functionally similar to your #40 anyway.

Should I try to build a bench case and open an issue in serde or serde_json?

@dtolnay
Copy link
Contributor

dtolnay commented Sep 4, 2018

Yes, please make a serde issue with a representative benchmark. Thanks!

@masklinn
Copy link
Author

masklinn commented Sep 9, 2018

@dtolnay opened serde-rs/serde#1381

@masklinn masklinn closed this Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants