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

Use simdjson to read WAT payloads #41

Open
sebastian-nagel opened this issue Mar 7, 2023 · 6 comments
Open

Use simdjson to read WAT payloads #41

sebastian-nagel opened this issue Mar 7, 2023 · 6 comments

Comments

@sebastian-nagel
Copy link
Contributor

Simdjson (pysimdjson) should be faster than ujson when parsing WAT payloads. Could be worth to use it as a drop-in replacement if installed (cf. #34 regarding ujson replacing the built-in json module).

@silentninja
Copy link
Contributor

Do we want to exclusively support pysimdjson, or should we consider implementing adapter classes to support multiple parsers? This would allow users to switch parsers at runtime, similar to what is proposed for HTML parsers in PR #47 .

@wumpus
Copy link
Member

wumpus commented Dec 30, 2024

As Sebastian alludes to by mentioning #34, we like having a fallback. I see that pysimdjson claims that it has a fallback internally, but you can still run into weirdnesses like a package lacking a wheel causing problems in CI.

@silentninja
Copy link
Contributor

silentninja commented Dec 31, 2024

@wumpus Thanks for the comment!

I wanted to add a key point that pysimdjson provides a highly performant API, as detailed here: pysimdjson Performance

These APIs, such as parse offer significant performance benefits by creating objects only during access. However, these optimizations are unavailable in drop-in replacement methods like simdjson.loads().

Summary of Options

  1. Compatibility-Focused Approach:

    • If compatibility is the primary goal with a slight performance boost, using the drop-in API (simdjson.loads()) makes sense.
  2. Aliasing Parse as Loads:

    • We could alias the parse function as loads to maintain compatibility. However, this feels hacky, for example:
         def get_json_parser():
             try:
                 import simdjson
                 parser = simdjson.Parser()
                 return parser.parse
             except ImportError:
                 import ujson
                 return ujson.loads
         
         # Sample usage
         loads = get_json_parser()
         
         # Now you can use `json_parser` to parse JSON data
         data = loads()
  3. Performance-Focused Approach:

    • If performance is the goal, it would be better to expose the choice of parser through a configuration option. This allows users to explicitly choose pysimdjson for its advanced APIs while acknowledging its limitations (e.g., Issue #72, where incorrect use of performant APIs can lead to penalties).

This way, we balance compatibility and performance while letting users decide what works best for their needs. I prefer Option 3 as it is a much cleaner approach if going for performance

Thoughts?

@sebastian-nagel
Copy link
Contributor Author

Hi @silentninja,

  1. ... compatibility is the primary goal

Yes. Looks like that simdjson does not support every combination of the matrix (OS, platform), see https://pysimdjson.tkte.ch/: Mac OS on ARM is not supported. This already happened in the past with ujson (#34). A working fall-back is always required.

  1. Aliasing Parse as Loads:

No. It adds extra complexity on every WAT record and does not the maximum performance, see the notes about re-using the simdjson parser.

  1. ... performance is the goal

This can still be achieved based on inheritance. For example, several example classes have a variant using FastWARC instead of warcio, see #37/#38. But if you want it simple or stay compatible, you can always use the classes based on warcio. Of course, with regard to simdjson, it only makes sense to implement a performant solution for classes which consume JSON resp. WAT files, and are used not only as simple example. So, I could imagine to implement it in ExtractHostLinksFastWarcJob because this class is used by Common Crawl every month to extract host-level links to span up the web graph.

@TkTech
Copy link

TkTech commented Jan 25, 2025

Yes. Looks like that simdjson does not support every combination of the matrix (OS, platform), see https://pysimdjson.tkte.ch/: Mac OS on ARM is not supported. This already happened in the past with ujson (#34). A working fall-back is always required.

Just noting that although it doesn't appear in the grid, it is supported and universal ARM/x86 wheels are published. Oversight on my part, grid will be updated with the next release.

If portability is your primary concern, https://github.com/tktech/py_yyjson performs much the same as pysimdjson while being standard C89, and has binary wheels available for all platforms with wheel tags.

See https://github.com/tktech/json_benchmark for comparisons of most popular parser.

@silentninja
Copy link
Contributor

silentninja commented Feb 13, 2025

After trying out simdjson, there are a few pitfalls that make me hesitant to use simdjson as a direct alternative to the standard json module.

  1. The API of the simdjson object is different from the object created by the json module. This can lead to errors like Check for None key fails when using parse TkTech/pysimdjson#122.

  2. Unlike the json module, the simdjson object requires explicit deallocation. If not properly managed, it can lead to errors, as it’s more sensitive to memory management.

  3. The parser returned by simdjson is not serializable, which can cause exceptions if not handled correctly.

While none of the mentioned issues don't break the existing functions but if we intent to go with simdjson, the tasks should be written primarily for the stricter simdjson while using json as a fallback. Is that acceptable for this project?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants