Skip to content
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

Observations #389

Open
1 of 9 tasks
Kriskras99 opened this issue Sep 9, 2024 · 1 comment
Open
1 of 9 tasks

Observations #389

Kriskras99 opened this issue Sep 9, 2024 · 1 comment

Comments

@Kriskras99
Copy link
Contributor

Kriskras99 commented Sep 9, 2024

These are some observations I've made while reading through the documentation and the code. These are not things that necessarily need to be resolved, but I wanted to write them down as they are my impressions from reading through the codebase for the first time.

  • There are a bunch of public macros and constants that seem like they belong in the private API (likely!, unlikely!, static_cast_*!, stry!, SIMDINPUT_LENGTH, SIMDJSON_PADDING)
  • OwnedValue could just be BorrowedValue with something like the ownable traits to remove the lifetime when necessary. And for to_owned_value can just return a BorrowedValue<'static>.
  • Experiment with replacing beef::lean::Cow with HipStr, which inlines strings up to 23 bytes
  • Is there a variant of the APIs that accept a &str or String as input? Redoing the UTF-8 validation seems like a waste (but I haven't checked the performance impact)
  • The various re-exports make the docs quite confusing, especially pub use crate::value::* which hides everything it re-exports behind a *
    • It might be better to make the re-exported modules private, which makes the re-exported types and functions appear as if they are really in the root
  • Prelude docs say Prelude to include needed traits for every trait and module
  • Prelude includes modules, which kinda defeats the purpose of the prelude (this is actually an issue in value-trait)
  • Is the project still tracking simdjson v0.2? Because they are currently at v3.10
  • lib.rs is filled with massive functions that should probably be in their own module

If anything here is wrong or for a reason I don't know, please correct me!

@Licenser
Copy link
Member

Licenser commented Sep 9, 2024

These are some observations I've made while reading through the documentation and the code.

  • There are a bunch of public macros and constants that seem like they belong in the private API (likely!, unlikely!, static_cast_*!, stry!, SIMDINPUT_LENGTH, SIMDJSON_PADDING)
    ja those could totally become private. iex #385 removes some of them too
  • OwnedValue could just be BorrowedValue with something like the ownable traits to remove the lifetime when necessary. And for to_owned_value can just return a BorrowedValue<'static>.

I'll have to read up on ownable but I am not sure that works since string parsing for owned values is different and I'm not aware of a way to have different implementations for a function solely based on lifetime.

  • Experiment with replacing beef::lean::Cow with HipStr, which inlines strings up to 23 bytes

that's worth benchmarking, but I don't have high hopes since COW right now uses a zero-copy approach, which will be more performant than inlining - it might also increase the size of the value struct, which would not be great.

  • Is there a variant of the APIs that accept a &str or String as input? Redoing the UTF-8 validation seems like a waste (but I haven't checked the performance impact)

there isn't, but there could be I guess, but it is questionable if it is faster, then we need to re-do utf8 validation as it should re-use the same registers as the parsing. That said it got crated out at one point so I am not 100% sure the compiler still does that - but if it doesn't it's a bug and should be fixed as one :)

  • The various re-exports make the docs quite confusing, especially pub use crate::value::* which hides everything it re-exports behind a *

    • It might be better to make the re-exported modules private, which makes the re-exported types and functions appear as if they are really in the root
      both makes sense 👍
  • Prelude docs say Prelude to include needed traits for every trait and module
  • Prelude includes modules, which kinda defeats the purpose of the prelude (this is actually an issue in value-trait)
    makes sense too :)
  • Is the project still tracking simdjson v0.2? Because they are currently at v3.10
    yes, upgrading to 3 is a quite big task since a lot of the internal have changed, I haven't found the time for that :(
  • lib.rs is filled with massive functions that should probably be in their own module
    I'm fine with that too

If anything here is wrong or for a reason I don't know, please correct me!

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

No branches or pull requests

2 participants