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

Convert the use of a Variable Length Array to a regular allocation #1542

Closed
wants to merge 3 commits into from

Conversation

nwc10
Copy link
Contributor

@nwc10 nwc10 commented Sep 17, 2021

This should restore the build on MSVC.

coke and others added 3 commits September 15, 2021 21:36
VLA fails on windows, this allows us to catch it on other platforms first.

Resolves #1537
Instead of allocating each time through the loop, keep a small array to
reuse each time through the loop. Expand the size of the array when
necessary.
The Fixed Size Allocator should be slightly faster than malloc for the
small allocations we need, and possibly much faster than free, because we
know the size of the memory we are deallocating.
@nwc10
Copy link
Contributor Author

nwc10 commented Sep 17, 2021

@jnthn are my assumptions about the FSA being a better fit reasonable?

I haven't actually benchmarked them :-)

@nwc10
Copy link
Contributor Author

nwc10 commented Sep 17, 2021

@coke I thought it clearer (for history) to merge my changes to your original commit into one change. It's not clearer for attribution - this is still fine with you?

@nwc10
Copy link
Contributor Author

nwc10 commented Sep 17, 2021

alloca would better.

@nwc10 nwc10 closed this Sep 17, 2021
@coke
Copy link
Collaborator

coke commented Sep 17, 2021

@nwc10 - sure. The config change was ok, but my code change was a WIP that needed fixing anyway, thanks!

@nwc10
Copy link
Contributor Author

nwc10 commented Sep 17, 2021

celo_duro suggested alloca. It seems the simplest approach - see #1543

@nwc10 nwc10 deleted the jit-vla-v2 branch September 17, 2021 14:10
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

3 participants