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

Added !defined(HB_NO_MT) to conditional compilation that does '#include <atomic>' #4119

Conversation

nwoods-cimpress
Copy link

The #include <atomic> declaration causes problems for WebAssembler, a single threaded platform. HarfBuzz already has a preexisting macro to disable multithreading, so we're using that.

…de <atomic>'

The `#include <atomic>` declaration causes problems for WebAssembler, a single threaded platform.  HarfBuzz already has a preexisting macro to disable multithreading, so we're using that.
@behdad
Copy link
Member

behdad commented Feb 14, 2023

This is problematic. The _hb_compiler_memory_r_barrier is misleadingly put in hb-atomic.hh. It's actually needed in single-threaded platforms as well, as it's a compiler memory barrier, not CPU memory barrier. The comment before it also says that it should not be disabled. We need to find another solution for your needs.

@behdad
Copy link
Member

behdad commented Feb 14, 2023

I think on gcc-like platforms we can define the compiler memory-barrier to asm volatile("": : :"memory").

@behdad
Copy link
Member

behdad commented Feb 14, 2023

I think on gcc-like platforms we can define the compiler memory-barrier to asm volatile("": : :"memory").

Or would that be even worse for WebAssembly?

@nwoods-cimpress
Copy link
Author

asm volatile("": : :"memory") seems to be fine
https://godbolt.org/z/TK9rEcGjT

behdad added a commit that referenced this pull request Feb 14, 2023
@behdad behdad closed this in #4120 Feb 14, 2023
behdad added a commit that referenced this pull request Feb 14, 2023
@nwoods-cimpress
Copy link
Author

Thanks, this looks good!

Just out of curiosity, why are memory fences needed at all in single threaded environments with HB_NO_MT defined? I'm guessing some esoteric environment, perhaps embedded?

@behdad
Copy link
Member

behdad commented Feb 14, 2023

See:
f3151b6

It's because we abuse the memory layout. and have a struct where accessing the second member can possibly cause a crash, so we need to disable compiler reordering access.

@nwoods-cimpress
Copy link
Author

I'm almost certainly preaching to the choir on this, but it feels unfortunate that we have this partial bounds checking (which if I'm reading things correctly, only catches scenarios where the offset is so large the pointer arithmetic overflows) and memory fences at all.

As an experiment, I tried disabling the memory fences with a gross hack (-D_hb_compiler_memory_r_barrier=__builtin_FILE) and found that my optimized WASM file became 1,505 bytes smaller. Not huge, but not zero and it was a larger savings than I expected. (And people always get on my case about WASM bloat :-))

In any case, thanks for all of your help!

@behdad
Copy link
Member

behdad commented Feb 17, 2023

Are you compiling HB_TINY profile?

@nwoods-cimpress
Copy link
Author

I am not; I looked at it a while ago and had concerns, the details of which I don't recall. I'll take a look again - thanks!

@behdad
Copy link
Member

behdad commented Feb 17, 2023

I am not; I looked at it a while ago and had concerns, the details of which I don't recall. I'll take a look again - thanks!

Or HB_MINI. Those can reduce the size to half or one-third... You might need to customize HB_TINY, but we are here to help.

@nwoods-cimpress
Copy link
Author

So I reviewed the literature for HB_MINI and HB_TINY and I remember my reasons for shying away from them. For the application that I'm working on, I really don't know much about the nitty gritty of fonts that I'm burdened with working with, and I got scared away by statements such as "[disabling] esoteric or rarely-used shaping features"

I like the idea of trimming down HarfBuzz to make it smaller, but not at the cost of not being able to handle something out there that might be esoteric, but in practice someone in the ether might want me to handle successfully. HarfBuzz has given me a luxury of ignorance about the details of fonts that I'm reluctant to give up.

Thanks!

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.

None yet

2 participants