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

Stack usage optimization for tostream_visitor #4182

Merged
merged 1 commit into from
Mar 19, 2022

Conversation

thehans
Copy link
Member

@thehans thehans commented Mar 19, 2022

This change was tested against the sample code in #4172, and while it does not fix the crash, it delays the crash until higher values of b, by going through fewer function calls, and re-using tostream_visitor instead of constructing new ones for each nested vector.

My testing showed the lowest value to trigger a crash went up by about 1000 after changes.
Lowest b value required to trigger a crash, built with g++-10

RelWithDebInfo build:
  unedited master: b=5390
  with changes:    b=6378
Release build:
  unedited master: b=4936
  with changes:    b=6081
Debug build:
  unedited master: b=3465
  with changes:    b=3905

Also removed some Value::toString|toStream functions which were not being used.

Also remove unused Value toString,toStream functions.
@MichaelPFrey
Copy link
Member

MichaelPFrey commented Mar 19, 2022

#4173 contains a similar approach partially implemented. This implementation here is a lot cleaner.

As I want to discard #4173 anyway to get a cleaner implementation, merging this here is welcome by me.

I think fixing #4172 (add stack check for vectors to toEchoString()) will also be its own merge request (before show usermodule parameters in trace).

Splitting it up makes things a lot easier.

@thehans thehans merged commit 398d59f into openscad:master Mar 19, 2022
@thehans
Copy link
Member Author

thehans commented Mar 19, 2022

Thanks for looking it over, I was hoping you would see this since it affects some of the related code you were working on, and alters the required b value for the test case by a significant amount.

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

Successfully merging this pull request may close these issues.

None yet

3 participants