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

Enhanced list indexing does not work in the background thread #215

Closed
olexandr-konovalov opened this issue Sep 16, 2015 · 4 comments
Closed
Assignees
Labels
topic: HPC-GAP Issues and PRs related to HPC-GAP

Comments

@olexandr-konovalov
Copy link
Member

This is a follow-up of PR #209 (see some comments at that PR). The new test listindex.tst added in PR #192 by @stevelinton works only in the main execution thread. Otherwise, there is no access to WRAPPER_OPERATIONS. Perhaps the solution is to make the latter an atomic list.

@olexandr-konovalov olexandr-konovalov added the topic: HPC-GAP Issues and PRs related to HPC-GAP label Sep 16, 2015
@stevelinton
Copy link
Contributor

That solution (making it an atomic list) seems appropriate. It is used only by adding entries and tested only in that one debugging warning. Even if there was a race condition between two threads it would simply result in a missed warning.

On the other hand, the new listindex.tst does something that no other test file does, as far as I know,
namely create a category and install a bunch of methods for it. So it would not be surprising if running it in the background thread throws up other problems.

@olexandr-konovalov
Copy link
Member Author

Yes - now it trips over OPERATIONS. That one is a list that stores all GAP operations at the odd positions, and the corresponding list of requirements at the even positions. Therefore, its dangerous to make it atomic list as this may cause other threads seeing incomplete state of this list. One option it to make it shared. The overhead should be tiny, since after the code is loaded, only readonly access will be needed to OPERATIONS. Another option would be to reformat OPERATIONS and make it an atomic list of lists of length two, but that would require larger rewrite, will divert codebases for GAP 4 and GAP 5, and may break some code relying on this format...

@stevelinton
Copy link
Contributor

Shared seems appropriate. CATS_AND_REPS and FILTERS which do a similar job in type.g is already shared. There might be a case for having a single OPERATIONS_REGION and putting OPERATIONS and WRAPPER_OPERATIONS and anything else of a similar nature in it.

Access is only while installing operations or methods, not while calling them, so there should be no performance cost once everything is loaded.

There are also places where two or more operations are added at once (eg attribute setters, getters and testers) and such things should probably be atomic.

@olexandr-konovalov
Copy link
Member Author

Fixed by PR #221.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: HPC-GAP Issues and PRs related to HPC-GAP
Projects
None yet
Development

No branches or pull requests

2 participants