Improve performance of the Julia GC integration #3516

rbehrends · 2019-06-21T13:15:49Z

This is both a performance fix and an attempt to refactor part of the Julia GC integration.

On the performance side, this avoids rescanning unused task stacks unnecessarily. See this related GAP.jl issue.

I've also attempted a migration of the GC integration to C++. The motivation for that was the need to have clean data types, which were messy to handle in C (especially the generics). This probably requires some testing.

Also, while GAP currently already uses some C++ code, it is not linked against the C++ stdlib, which means that things such as new and delete won't properly work. Therefore, the C++ is written to work without any such features and is primarily used for abstraction. Care was also taken to only require C++98 features (build with CXXFLAGS="-g -O2 -std=c++98" to verify).

There's currently a short test, build with CXXFLAGS=-DTEST_JULIA_GC_INTERNALS to run it instead of GAP. This is only temporary and meant to be removed before this PR is being merged.

Note that the C++ stdlib issues could be resolved by using CXX instead of CC for the LINK command in Makefile.rules, but I am not sure if that is desirable.

src/julia_gc.cc

codecov · 2019-06-23T03:31:20Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2e9ace1). Click here to learn what that means.
The diff coverage is 42.78%.

@@            Coverage Diff            @@
##             master    #3516   +/-   ##
=========================================
  Coverage          ?   84.48%           
=========================================
  Files             ?      698           
  Lines             ?   345258           
  Branches          ?        0           
=========================================
  Hits              ?   291695           
  Misses            ?    53563           
  Partials          ?        0

Impacted Files	Coverage Δ
src/dynarray.h	`14.54% <14.54%> (ø)`
src/baltree.h	`40.78% <40.78%> (ø)`
src/julia_gc.c	`82.44% <69.84%> (ø)`

coveralls · 2019-06-23T04:23:34Z

Coverage decreased (-0.02%) to 84.3% when pulling 7431c7c on rbehrends:julia-gc-perf-quickfix into 2e9ace1 on gap-system:master.

src/julia_gc.cc

fingolfin · 2019-06-25T14:14:05Z

src/julia_gc.cc

+    if (!FullGC) {
+        // This is a temp hack to work around a problem with the
+        // generational GC. Basically, task stacks are being scanned
+        // regardless of whether they are old or new. In order to avoid


Is this a bug in Julia, resp. something there we don't understand? If so, can you please provide a link to the relevant Julia issue report, resp. the relevant discussion on the Julia discourse?

I'm not sure if its a bug or an inefficiency. Basically, as I understand it, the task stacks can be roots themselves without being referenced from elsewhere. So, in parts, this is most likely intentional and simply hasn't created any performance issues, as task stacks tend to be short.

src/julia_gc.cc

fingolfin · 2019-06-27T21:37:57Z

@rbehrends this version doesn't compile cleanly, at least on Travis.

rbehrends · 2019-06-27T23:57:16Z

@fingolfin Yeah, I saw it, for some reason clang incorrectly accepted a static const array size (a C++ leftover), but gcc complained (appropriately so).

fingolfin

In principle I am fine with this now.

fingolfin · 2019-07-15T13:33:17Z

src/dynarray.h

+#define FN(sym) JOIN(Array, sym)
+
+#define JOIN(s1, s2) JOIN2(s1, s2)
+#define JOIN2(s1, s2) s1##s2


These two macros also appear in sortbase.h. And in debug.h, we have them under a different name:

#define _intern_CONCAT_(X, Y) X ## Y #define _intern_CONCAT(X, Y) _intern_CONCAT_(X,Y)

Perhaps we should just give in and move this to, say, src/gaputils.h and document it properly (this is not a change request for this PR, just a general thought. @ChrisJefferson ?)

rbehrends · 2019-07-16T05:52:40Z

I've factored out the tests and made testkernel a separate Makefile target under the assumption that more could be added to it later and kept conceptually different from libgap tests, even though the same framework is used. Is that reasonable or should I put them in with the libgap tests?

Makefile.rules

src/julia_gc.c

This introduces a new make target, testkernel.

fingolfin reviewed Jun 21, 2019

View reviewed changes

src/julia_gc.cc Outdated Show resolved Hide resolved

fingolfin requested changes Jun 25, 2019

View reviewed changes

ChrisJefferson approved these changes Jun 26, 2019

View reviewed changes

src/julia_gc.cc Outdated Show resolved Hide resolved

src/julia_gc.cc Outdated Show resolved Hide resolved

rbehrends force-pushed the julia-gc-perf-quickfix branch 2 times, most recently from 47324a9 to 3688ac0 Compare June 27, 2019 17:39

fingolfin approved these changes Jul 15, 2019

View reviewed changes

rbehrends changed the title ~~WIP: Performance fix for the Julia GC integration.~~ Performance fix for the Julia GC integration. Jul 15, 2019

fingolfin reviewed Jul 17, 2019

View reviewed changes

rbehrends added 2 commits July 17, 2019 12:45

Performance fix for the Julia GC.

d4c729d

Factor out setjmp() code to avoid volatile casting hell.

b7a076b

fingolfin force-pushed the julia-gc-perf-quickfix branch from 4cc0bac to a31119c Compare July 17, 2019 10:46

Refactor Julia GC integration tests.

7431c7c

This introduces a new make target, testkernel.

fingolfin force-pushed the julia-gc-perf-quickfix branch from a31119c to 7431c7c Compare July 17, 2019 12:56

fingolfin merged commit f33b341 into gap-system:master Jul 17, 2019

fingolfin added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) labels Aug 21, 2019

fingolfin changed the title ~~Performance fix for the Julia GC integration.~~ Fix performance issues in the Julia GC integration Aug 21, 2019

fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019

fingolfin changed the title ~~Fix performance issues in the Julia GC integration~~ Improve performance of the Julia GC integration Aug 22, 2019

olexandr-konovalov added this to the GAP 4.11.0 milestone Aug 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of the Julia GC integration #3516

Improve performance of the Julia GC integration #3516

rbehrends commented Jun 21, 2019 •

edited

Loading

codecov bot commented Jun 23, 2019 •

edited

Loading

coveralls commented Jun 23, 2019 •

edited

Loading

fingolfin Jun 25, 2019

rbehrends Jun 27, 2019

fingolfin commented Jun 27, 2019

rbehrends commented Jun 27, 2019

fingolfin left a comment

fingolfin Jul 15, 2019

rbehrends commented Jul 16, 2019

Improve performance of the Julia GC integration #3516

Improve performance of the Julia GC integration #3516

Conversation

rbehrends commented Jun 21, 2019 • edited Loading

codecov bot commented Jun 23, 2019 • edited Loading

Codecov Report

coveralls commented Jun 23, 2019 • edited Loading

fingolfin Jun 25, 2019

Choose a reason for hiding this comment

rbehrends Jun 27, 2019

Choose a reason for hiding this comment

fingolfin commented Jun 27, 2019

rbehrends commented Jun 27, 2019

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Jul 15, 2019

Choose a reason for hiding this comment

rbehrends commented Jul 16, 2019

rbehrends commented Jun 21, 2019 •

edited

Loading

codecov bot commented Jun 23, 2019 •

edited

Loading

coveralls commented Jun 23, 2019 •

edited

Loading