Rework memory manager to make stack slot update faster #2813

olonho · 2019-03-25T11:37:55Z

No description provided.

SvyatoslavScherbina · 2019-03-27T13:25:39Z

runtime/src/main/cpp/Memory.h

@@ -455,6 +466,8 @@ extern "C" {
    ObjHeader* result = name(__VA_ARGS__, OBJ_RESULT);  \
    return result;                                      \
  }
+#define HEAP_RETURN_SLOT(slot) \


Why not enforce return slot to be always stack-located instead?
I believe that this is

possible

simpler

more efficient

It's not just stack allocated, it must be in slots seen when walking the stack. Let's first stabilize this design and see its performance implications and the proceed with optimisations.

Did that help?

SvyatoslavScherbina · 2019-04-01T12:00:13Z

runtime/src/main/cpp/Memory.cpp

    // We do not use cycle collector for frozen objects, as we already detected
    // possible cycles during freezing.
    // Also do not use cycle collector for provable acyclic objects.
    int color = container->color();
    if (color != CONTAINER_TAG_GC_PURPLE && color != CONTAINER_TAG_GC_GREEN) {
+      RuntimeAssert(!container->shareable(), "Shareable cannot be cycle collected");


Isn't it the same as checked above?

SvyatoslavScherbina · 2019-04-01T12:32:21Z

runtime/src/main/cpp/Memory.cpp

 inline void initThreshold(MemoryState* state, uint32_t gcThreshold) {
  state->gcThreshold = gcThreshold;
-  state->toFree->reserve(gcThreshold);
+  state->toRelease->reserve(gcThreshold);


Why was toFree->reserve(gcThreshold) removed?

Do you suppose that it is unlikely to get filled up to threshold because toRelease will become full earlier?

Yes, it's no longer even close to that threshold.

SvyatoslavScherbina · 2019-04-01T12:45:36Z

runtime/src/main/cpp/Memory.cpp

  header_ = AllocContainer(alloc_size);
+  RuntimeCheck(header_ != nullptr, "Cannot alloc memory");


So the check below isn't required anymore?

SvyatoslavScherbina · 2019-04-01T13:25:57Z

runtime/src/main/cpp/Memory.cpp


 #if GC_ERGONOMICS
  auto gcStartTime = konan::getTimeMicros();
 #endif

  state->gcInProgress = true;

+  incrementStack(state);


Do I understand correctly that long-living stack references become cycle candidates during every GC?

Exactly. Any alternatives?

Not yet, but I believe we should eventually apply some efforts to find some.

SvyatoslavScherbina · 2019-04-01T13:27:45Z

runtime/src/main/cpp/Memory.cpp

@@ -1638,8 +1800,7 @@ void Kotlin_native_internal_GC_resume(KRef) {
  MemoryState* state = memoryState;
  if (state->gcSuspendCount > 0) {
    state->gcSuspendCount--;
-    if (state->toFree != nullptr &&
-        freeableSize(state) >= state->gcThreshold) {
+    if (state->toFree != nullptr && freeableSize(state) >= state->gcThreshold) {


Btw, I guess gcSuspendCount == 0 should be checked here too.

SvyatoslavScherbina · 2019-04-01T13:31:40Z

runtime/src/main/cpp/Memory.cpp

@@ -1650,6 +1811,7 @@ void Kotlin_native_internal_GC_stop(KRef) {
 #if USE_GC
  if (memoryState->toFree != nullptr) {
    GarbageCollect();
+    // TODO: what shall we do with toRelease here? Just leak?


Btw, this GC.start/stop feature doesn't seem to work properly.
Consider e.g.

kotlin.native.internal.GC.stop() Any().freeze()

Yeah, more testing is needed, not sure if in this PR.

SvyatoslavScherbina · 2019-04-01T15:37:44Z

runtime/src/main/cpp/Worker.cpp

+      }
+      // Notify the future.
+      job.future->storeResultUnlocked(result, ok);
+      resultHolder.clear();


As far as I see, by this moment another thread may have got this object already, so clearing the holder here may cause data race.

Btw, can transfer have ObjHolder parameter instead and clear it by itself? This looks making sense to me.

SvyatoslavScherbina · 2019-04-04T07:33:40Z

runtime/src/main/cpp/Memory.cpp

+  if (stackReferences * 5 > state->gcThreshold) {
+     auto newThreshold = state->gcThreshold * 3 / 2 + 1;
+     if (newThreshold < kMaxErgonomicThreshold) {
+       state->gcThreshold = newThreshold;


Is space in toRelease not reserved intentionally?

Reworked here.

SvyatoslavScherbina · 2019-04-04T07:35:31Z

runtime/src/main/cpp/Memory.cpp

    memoryState->toFree = nullptr;
    memoryState->roots = nullptr;
+    state->gcSuspendCount = 0;


What if GC is stopped while suspended?

Hard to tell what is right, but moved zeroing above.

SvyatoslavScherbina · 2019-04-05T09:53:13Z

runtime/src/main/cpp/Memory.cpp

+          state->toFree->push_back(container);
+          if (state->gcSuspendCount == 0 && state->toFree->size() >= state->gcThreshold) {
+              GC_LOG("Calling GC from DecrementRC: %d\n", state->toRelease->size())
+              garbageCollect(state, false);


This DecrementRC variant is used only during GC. What's the purpose of invoking GC inside it?

SvyatoslavScherbina · 2019-04-08T07:19:27Z

runtime/src/main/cpp/Memory.cpp

@@ -111,9 +111,10 @@ volatile int allocCount = 0;
 volatile int aliveMemoryStatesCount = 0;

 // Forward declarations.
+__attribute__((noinline))


NO_INLINE?

SvyatoslavScherbina · 2019-04-08T07:24:25Z

runtime/src/main/cpp/Memory.cpp

-    if (container->hasContainerSize() && container->containerSize() == size) {
+    // TODO: shall it be == instead?
+    if (container->hasContainerSize() &&
+        container->containerSize() >= size && container->containerSize() <= 2 * size) {


container->containerSize() <= 2 * size is too permissive. Can it be more like container->containerSize() <= size + 8?

SvyatoslavScherbina · 2019-04-08T07:27:14Z

runtime/src/main/cpp/Memory.cpp

-    if (old != nullptr ) {
-      ReleaseStackRef(old);
-    }
+  if (object != nullptr) {


Does removing old != object check have significant positive effect?

Just remove excessive branch, but can return.

SvyatoslavScherbina · 2019-04-09T07:12:11Z

runtime/src/main/cpp/Memory.h

 // Class holding reference to an object, holding object during C++ scope.
 class ObjHolder {
 public:
-   ObjHolder() : obj_(nullptr) {}
+   ObjHolder() : obj_(nullptr) {


So it is actually now more like ObjStackHolder, StackRefHolder or even StackFrame.

SvyatoslavScherbina · 2019-04-09T12:47:15Z

runtime/src/main/cpp/Worker.cpp

@@ -62,10 +62,11 @@ enum {
 THREAD_LOCAL_VARIABLE KInt g_currentWorkerId = 0;

 KNativePtr transfer(ObjHolder* holder, KInt mode) {
+  holder->hold();


Why not just

void* result = CreateStablePointer(holder.obj()) if (!ClearSubgraphReferences(holder->obj(), mode == CHECKED)) { ThrowWorkerInvalidState(); } holder.clear() return result;

Technically the same, but avoids leaking abstractions and pairs with AdoptStablePointer semantically.

…ryState.

olonho force-pushed the cheaper_stack branch from b8e6a10 to 3fd7d8c Compare March 27, 2019 08:53

SvyatoslavScherbina reviewed Mar 27, 2019

View reviewed changes

olonho force-pushed the cheaper_stack branch from 3fd7d8c to b076507 Compare March 27, 2019 19:20

olonho changed the title ~~Cheaper stack~~ Rework memory manager to make stack slot update faster Mar 27, 2019

olonho force-pushed the cheaper_stack branch 3 times, most recently from d28e986 to 309c62e Compare April 1, 2019 11:20

SvyatoslavScherbina reviewed Apr 1, 2019

View reviewed changes

olonho force-pushed the cheaper_stack branch from ee29898 to 74c1a6d Compare April 3, 2019 14:02

SvyatoslavScherbina reviewed Apr 4, 2019

View reviewed changes

SvyatoslavScherbina reviewed Apr 5, 2019

View reviewed changes

SvyatoslavScherbina reviewed Apr 8, 2019

View reviewed changes

olonho force-pushed the cheaper_stack branch 2 times, most recently from 21970da to d7dd6d2 Compare April 9, 2019 07:09

SvyatoslavScherbina approved these changes Apr 9, 2019

View reviewed changes

SvyatoslavScherbina reviewed Apr 9, 2019

View reviewed changes

olonho force-pushed the cheaper_stack branch from 87521e9 to 87cc869 Compare April 9, 2019 11:07

SvyatoslavScherbina reviewed Apr 9, 2019

View reviewed changes

olonho force-pushed the cheaper_stack branch from 36bd844 to 932d1f4 Compare April 9, 2019 13:22

SvyatoslavScherbina approved these changes Apr 9, 2019

View reviewed changes

Faster stack slot management in runtime.

15a0941

Nikolay Igotti and others added 22 commits April 9, 2019 15:39

Temporary disable stacktrace format test.

da6547f

Fix interop weak reference test.

11df923

Fix remaining issues with transfer and frozen objects.

ef06811

Fixed frozen object rc actualization.

625b292

Fix logging.

16e35c9

WIP

4f0b4d4

Fix worker11 test.

26abf0f

Store current frame in top level thread local, to avoid need for memo…

f628041

…ryState.

Review feedback, more logging.

5c1fbac

Fix typo.

faf5e50

Review feedback, some optimizations of GC.

3579521

Refactor memory manager, recycle elements of finalizer queue.

b2a0452

Fix stupid typo.

792561a

Review feedback, increase finalizer queue size.

d38bdf9

Avoid calling Any constructor, avoid uneeded retention.

ec377f8

Optimize object management.

d820db5

Perf tweak.

29d2e86

Fix problem with reference counting of stack objects.

323fd08

Switch to always-stack return slot.

805b2cb

Review feedback.

8499958

Properly handle tricky cases of transfer.

98aa11e

Review feedback.

30be336

olonho force-pushed the cheaper_stack branch from 932d1f4 to 30be336 Compare April 9, 2019 13:39

olonho merged commit d072920 into master Apr 9, 2019

olonho deleted the cheaper_stack branch April 9, 2019 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework memory manager to make stack slot update faster #2813

Rework memory manager to make stack slot update faster #2813

olonho commented Mar 25, 2019

SvyatoslavScherbina Mar 27, 2019

olonho Mar 27, 2019

olonho Apr 9, 2019

SvyatoslavScherbina Apr 9, 2019

SvyatoslavScherbina Apr 1, 2019

olonho Apr 3, 2019

SvyatoslavScherbina Apr 1, 2019

SvyatoslavScherbina Apr 1, 2019

olonho Apr 1, 2019

SvyatoslavScherbina Apr 1, 2019

SvyatoslavScherbina Apr 1, 2019

olonho Apr 3, 2019

SvyatoslavScherbina Apr 4, 2019

SvyatoslavScherbina Apr 1, 2019

SvyatoslavScherbina Apr 1, 2019

olonho Apr 3, 2019

SvyatoslavScherbina Apr 1, 2019

SvyatoslavScherbina Apr 4, 2019

olonho Apr 5, 2019

SvyatoslavScherbina Apr 4, 2019

olonho Apr 5, 2019

SvyatoslavScherbina Apr 5, 2019

SvyatoslavScherbina Apr 8, 2019

SvyatoslavScherbina Apr 8, 2019

SvyatoslavScherbina Apr 8, 2019

olonho Apr 9, 2019

SvyatoslavScherbina Apr 9, 2019

SvyatoslavScherbina Apr 9, 2019 •

edited

Loading

		header_ = AllocContainer(alloc_size);
		RuntimeCheck(header_ != nullptr, "Cannot alloc memory");

Rework memory manager to make stack slot update faster #2813

Rework memory manager to make stack slot update faster #2813

Conversation

olonho commented Mar 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SvyatoslavScherbina Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

SvyatoslavScherbina Apr 9, 2019 •

edited

Loading