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

Cleanup stacks in pc group collection code #2461

Merged
merged 3 commits into from
May 22, 2018

Conversation

fingolfin
Copy link
Member

The first commit is motivated by the Julia GC work: the classic pc group collected code in the kernel (ab)used T_PLIST_EMPTY resp. T_STRING objects to store various stacks, but overwrote the length slot with garbage in the process. We change it to use a T_DATOBJ, and take care not to overwrite its type slot.

The second commit adds in-kernel stacks for the "new" pc(p) group collector, used by the polycyclic package. Right now, every pcp group collector has its own set of stacks, resulting in collector objects which are several hundred kilobytes big. This is a crazy waste if you have many collectors around.

TODO: more tests; and in order to allow polycyclic to smoothly transition away from the stacks-in-collectors, we need to add a way for it to find out when it can do that. I guess I'll just add a constant NO_STACKS_INSIDE_COLLECTORS or so, and then make a polycyclic release using it to disable the stack if it is set to true. And once that happened, we can think about removing the relevant constants like PC_WORD_EXPONENT_STACK from the kernel...

@fingolfin fingolfin added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels May 13, 2018
@codecov
Copy link

codecov bot commented May 15, 2018

Codecov Report

Merging #2461 into master will increase coverage by <.01%.
The diff coverage is 73.91%.

@@            Coverage Diff             @@
##           master    #2461      +/-   ##
==========================================
+ Coverage   74.19%   74.19%   +<.01%     
==========================================
  Files         484      484              
  Lines      245306   245311       +5     
==========================================
+ Hits       181996   182011      +15     
+ Misses      63310    63300      -10
Impacted Files Coverage Δ
src/objscoll.c 66.39% <100%> (+0.27%) ⬆️
src/objcftl.c 24.3% <61.9%> (+6.07%) ⬆️
src/objccoll-impl.h 94.14% <68.75%> (+2.48%) ⬆️
src/objscoll-impl.h 73.59% <68.75%> (+1.67%) ⬆️
hpcgap/lib/hpc/stdtasks.g 63.42% <0%> (-0.77%) ⬇️
src/hpc/threadapi.c 43.59% <0%> (+0.1%) ⬆️

@fingolfin fingolfin changed the title WIP: Cleanup stacks in pc group collection code Cleanup stacks in pc group collection code May 15, 2018
@fingolfin fingolfin removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label May 15, 2018
Otherwise, if a garbage collection happens after we assign a new bag to
<bag>, but before calling InitGlobalBag on it, the new bag may be garbage
collected, and we end up with a bad reference.
These stacks were created as T_PLIST_EMPTY, but then used as if they were
T_STRING (but with the length field overwritten with arbitrary data), then
were resized into T_STRING.

Now they are T_DATOBJ, and we make sure not to overwrite the type slot.
... instead of using the stack objects in the pcp collector objects provided
by the polycyclic package. This will make it possible to remove the latter in
a future version of polycyclic, thus reducing the size of these objects
considerably.
@fingolfin fingolfin merged commit d6d1b20 into gap-system:master May 22, 2018
@fingolfin fingolfin deleted the mh/collector-stacks branch May 22, 2018 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants