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

JUCE suggestions #60

Merged
merged 4 commits into from
Oct 30, 2024
Merged

Conversation

Anthony-Nicholls
Copy link
Contributor

These commits are designed to prevent JUCE needing to add any additional workarounds when including choc.

The most controversial of which I suspect will be the two changes required to make the MSVC code analyser happy but I was keen to not simply disable the warnings using pragmas, add heap allocation where there wasn't already heap allocation, or add breaking changes (which I believe I've avoided).

@julianstorer
Copy link
Collaborator

Thanks Anthony - those all look great, except for the thread-local change. That function needs to be fast and realtime-safe, which is why it uses a local array rather than falling back to the heap if the size is too big. I don't think it'd be safe to trust a thread-local to be lock-free (or to be fast enough). Not sure what to suggest instead.. What circumstances did you see a problem?

@Anthony-Nicholls
Copy link
Contributor Author

Anthony-Nicholls commented Oct 29, 2024

I thought that might be the case.

So the warning we see is

...\choc_Value.h(2423): warning C6262: Function uses '17464' bytes of stack:  exceeds /analyze:stacksize '16384'.  Consider moving some data to heap.

This appears when code analysis is enabled on MSVC (https://learn.microsoft.com/en-us/cpp/build/reference/analyze-code-analysis). We could forcibly increase the stack size as an argument to analyse but 16384 is the default value.

Other options are

  • Decrease the value of maximumSize to 15304
  • Disable the warning with a pragma like so
#if _MSC_VER
 #pragma warning (push)
 #pragma warning (disable: 6262)
#endif

template <typename OutputStream>
void ValueView::serialise (OutputStream& output) const

#if _MSC_VER
 #pragma warning (pop)
#endif

Alternatively we can just wrap CHOC disabling the warning.

@julianstorer
Copy link
Collaborator

Hmm. The number I picked is pretty arbitrary, it could be smaller.

Or.. perhaps using alloca would get around it?

@Anthony-Nicholls
Copy link
Contributor Author

That might work! It has the nice bonus of being able to allocate only the amount requested based on the actual data size required too.

It's a little messy but this appears to be compiling on MSVC, untested otherwise though...

    ...
    check (dataSize > 0, "Invalid data size");

    uint8_t* localCopy = nullptr;

   #if _MSC_VER
    __try
    {
        localCopy = (uint8_t*) _alloca (dataSize);
    }
    __except (GetExceptionCode() == STATUS_STACK_OVERFLOW)
    {
        throwError ("Stack overflow");
    }
   #else
    localCopy = (uint8_t*) alloca (dataSize);
   #endif

    check (localCopy != nullptr, "Stack allocation failed");
    std::memcpy (localCopy, data, dataSize);
    ...

@julianstorer julianstorer changed the base branch from main to develop October 30, 2024 13:37
@julianstorer julianstorer merged commit 60d5327 into Tracktion:develop Oct 30, 2024
4 checks passed
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