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

move photonics service object inside traysegment so that staticmethod… #141

Closed
wants to merge 11 commits into from

Conversation

tianluyuan
Copy link
Contributor

…s can be invoked without overhead

fixing the issue discovered in #140

@kjmeagher
Copy link
Member

I don't think that this is the best way to fix this if we want to use shared memory for photonics. I think the best way would be to solve it the same way I solved it in #140 passing around the instance of recos.RecoInterface instead of the reco's name.

@tianluyuan
Copy link
Contributor Author

I do like the switch to passing reco interface objects, but I think these two PRs are not mutually exclusive. I think it depends on if or how we want to utilize the shared memory feature in photonics-service.

This makes it so that we're not creating large memory structures until they're actually needed, and the shared memory feature should still allow processes within the same container to access the same object once the first one is created.

@dsschult
Copy link
Member

dsschult commented Mar 3, 2023

I think @kjmeagher's solution is to avoid trusting boost::interprocess locking and only touch the locking code once. The tables are first created, then the handle to them is copied to each subprocess.

Vs in this PR, the locking gets activated in every subprocess to obtain access to the tables. So it's potentially more likely to have problems (if in fact there is a problem in the locking).

@tianluyuan
Copy link
Contributor Author

Closing for now as it sounds like photospline may remove the shared memory feature in the future.

@tianluyuan tianluyuan closed this Mar 25, 2023
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.

4 participants