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

WIP - Replace boost memory-mapped files with own implementation. #387

Closed
wants to merge 2 commits into from

Conversation

junhaoliao
Copy link
Member

References

When compiling the Zstd decompressor code with Emscripten for use in Log Viewer WebUI, it was identified that the code has dependency on Boost's memory-mapped files. That requires setting up extra compiling procedures - downloading Boost and also making modifications in Boost to avoid emcc complaining on missing some std::atomic implementations. However, the Boost dependencies can be replace with POSIX system calls.

In Emscripten compiled example code, with the replacement the speedup is > 30 ms.
 

Description

This is WIP. Criticism is welcomed:

  1. Shall we abstract the changes into a class to avoid bloating the Decoder class with 3 extra members?
  2. Shall we replace other Boost memory-mapped file usages in CLP core?

  1. Replace boost memory-mapped files with own implementation.

Validation performed

  1. Built and ran unitTest target and passed all:
    ...
    All tests passed (95962 assertions in 51 test cases)
    

Comment on lines +128 to +129
size_t m_compressed_file_size;
char* m_mem_mapped_compressed_file_buffer;
Copy link
Member

Choose a reason for hiding this comment

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

We should use std::span (probably wrapping with std::optional) instead

@kirkrodrigues
Copy link
Member

  1. Shall we abstract the changes into a class to avoid bloating the Decoder class with 3 extra members?
  2. Shall we replace other Boost memory-mapped file usages in CLP core?

If we do (2), then we should do (1). I think it would be a good idea to do both, but if we're time constrained, I think we could go with the current approach. If so, we can convert this to non-WIP and do a proper review.

@LinZhihao-723
Copy link
Member

This PR has been formally implemented in #445

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.

3 participants