Skip to content

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented Oct 27, 2024

Fixes #2057

Copy link

@oliverlee oliverlee left a comment

Choose a reason for hiding this comment

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

Nice!

Does this PR add a regression test?

aligned_storage is deprecated and should never be used. You can use an anonymous union with a single type instead.
https://stackoverflow.com/questions/71828288/why-is-stdaligned-storage-to-be-deprecated-in-c23-and-what-to-use-instead

@bjodah
Copy link
Contributor

bjodah commented Oct 27, 2024

For regression testing, we can replace our use of expand1.cpp here:

cp $SOURCE_DIR/benchmarks/expand1.cpp .

with a .cpp based on @oliverlee's example in gh-2057 perhaps?

@certik
Copy link
Contributor

certik commented Oct 27, 2024

So the way this works is:

  • Every translation unit gets its own constantInitializer
  • When the translation unit gets created (in any order), the constantInitializer increments a global counter
  • The global symengine constants are now references, and we allocate them exactly once, using the global counter
  • If constantInitializer's destructor ever gets called (not sure if that happens?), we free the global symengine expressions, exactly once.

I think this can work! Thanks @isuruf for implementing it.

@isuruf isuruf requested review from certik and bjodah October 28, 2024 16:02
Copy link
Contributor

@bjodah bjodah left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this looks good. Is my understanding at #2059 (comment) correct?

@isuruf
Copy link
Member Author

isuruf commented Oct 28, 2024

Is my understanding at #2059 (comment) correct?

Yep.

(not sure if that happens?)

It gets called when each translation unit gets unloaded and when all translation units are unloaded, the expression is freed. This happens when the program terminates or if we opened the library using dlopen, it happens when the library is closed via dlclose.

@certik
Copy link
Contributor

certik commented Oct 28, 2024

@isuruf excellent. (I wasn't sure if this happens when the program terminates, since the OS cleans the memory anyway.)

@isuruf isuruf merged commit d51595b into symengine:master Oct 28, 2024
36 checks passed
@isuruf isuruf deleted the siof branch October 28, 2024 20:40
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.

rcp nullptr assertion failure with global constants
4 participants