Cosmetic change to distinguish stack/heap writes and collect stats. #2794

olonho · 2019-03-20T16:52:48Z

No description provided.

… and print stats.

SvyatoslavScherbina · 2019-03-21T13:18:22Z

...backend.native/src/org/jetbrains/kotlin/backend/konan/llvm/objcexport/BlockPointerSupport.kt

@@ -63,7 +63,9 @@ internal class BlockAdapterToFunctionGenerator(val objCExportCodeGenerator: ObjC
    ) {
        val blockPtr = bitcast(pointerType(blockLiteralType), param(0))
        val slot = structGep(blockPtr, 1)
-        storeAny(kNullObjHeaderPtr, slot) // TODO: can dispose_helper write to the block?
+        // Although value is actually on the stack, it's not in normal slot area, so we cannot handle it


Although value is actually on the stack

AFAIK this is not correct.

SvyatoslavScherbina · 2019-03-21T13:19:46Z

...tive/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/llvm/CodeGenerator.kt

@@ -281,9 +281,13 @@ internal class FunctionGenerationContext(val function: LLVMValueRef,
        LLVMBuildStore(builder, value, ptr)
    }

-    fun storeAny(value: LLVMValueRef, ptr: LLVMValueRef) {
+    fun storeRef(value: LLVMValueRef, ptr: LLVMValueRef, onStack: Boolean) {


As far as I see, most of usages pass boolean literal for onStack parameter.
Why not add then storeHeapRef and storeStackRef to make the code more clear?

SvyatoslavScherbina · 2019-03-21T13:21:22Z

...backend.native/src/org/jetbrains/kotlin/backend/konan/llvm/objcexport/BlockPointerSupport.kt

@@ -84,7 +86,7 @@ internal class BlockAdapterToFunctionGenerator(val objCExportCodeGenerator: ObjC
        // Kotlin reference was `memcpy`ed from src to dst, "revert" this:
        storeRefUnsafe(kNullObjHeaderPtr, dstSlot)
        // and copy properly:
-        storeAny(loadSlot(srcSlot, isVar = false), dstSlot)
+        storeRef(loadSlot(srcSlot, isVar = false), dstSlot, true)


Is it a stack slot? I'm not sure it is even located on the stack.

SvyatoslavScherbina · 2019-03-21T13:23:50Z

...backend.native/src/org/jetbrains/kotlin/backend/konan/llvm/objcexport/BlockPointerSupport.kt

@@ -191,7 +193,9 @@ internal class BlockAdapterToFunctionGenerator(val objCExportCodeGenerator: ObjC
            val slot = structGep(blockOnStack, 1)

            listOf(bitcast(int8TypePtr, isa), flags, reserved, invoke, descriptor).forEachIndexed { index, value ->
-                storeAny(value, structGep(blockOnStackBase, index))
+                // Although value is actually on the stack, it's not in normal slot area, so we cannot handle it


I don't think any of this values is an object reference.

SvyatoslavScherbina · 2019-03-21T13:32:27Z

runtime/src/main/cpp/Memory.cpp

@@ -606,13 +612,13 @@ template <bool Atomic>
 inline void IncrementRC(ContainerHeader* container) {
  container->incRefCount<Atomic>();
  container->setColorUnlessGreen(CONTAINER_TAG_GC_BLACK);
-  UPDATE_ADDREF_STAT(memoryState, container, Atomic);
+  UPDATE_ADDREF_STAT(memoryState, container, Atomic, 0);


Is passing 0 correct?

olonho · 2019-03-21T16:55:54Z

Stats from ring benchmark execution, just for records:

Memory manager statistic:

Container alloc: 376838158, free: 376838158
Object normal alloc: 71258942
Object frozen alloc: 305579216

Total updates: 11243537129, stack: 10857499609(96.57%), heap: 386037520(3.43%)
UpdateHeapRef[normal -> normal]: 3038307 (0.03% of all, 0.79% of heap)
UpdateHeapRef[normal -> null  ]: 90670998 (0.81% of all, 23.49% of heap)
UpdateHeapRef[perm   -> perm  ]: 2962581 (0.03% of all, 0.77% of heap)
UpdateHeapRef[perm   -> frozen]: 296338 (0.00% of all, 0.08% of heap)
UpdateHeapRef[perm   -> null  ]: 18669083 (0.17% of all, 4.84% of heap)
UpdateHeapRef[frozen -> perm  ]: 5 (0.00% of all, 0.00% of heap)
UpdateHeapRef[frozen -> frozen]: 7005789 (0.06% of all, 1.81% of heap)
UpdateHeapRef[frozen -> null  ]: 75528734 (0.67% of all, 19.57% of heap)
UpdateHeapRef[null   -> normal]: 90974393 (0.81% of all, 23.57% of heap)
UpdateHeapRef[null   -> perm  ]: 18965415 (0.17% of all, 4.91% of heap)
UpdateHeapRef[null   -> frozen]: 75232248 (0.67% of all, 19.49% of heap)
UpdateHeapRef[null   -> null  ]: 2693629 (0.02% of all, 0.70% of heap)
UpdateStackRef[normal -> normal]: 1206043164 (10.73% of all, 11.11% of stack)
UpdateStackRef[normal -> perm  ]: 12 (0.00% of all, 0.00% of stack)
UpdateStackRef[normal -> frozen]: 14 (0.00% of all, 0.00% of stack)
UpdateStackRef[normal -> null  ]: 3256034479 (28.96% of all, 29.99% of stack)
UpdateStackRef[perm   -> normal]: 10 (0.00% of all, 0.00% of stack)
UpdateStackRef[perm   -> perm  ]: 388994921 (3.46% of all, 3.58% of stack)
UpdateStackRef[perm   -> frozen]: 2947776 (0.03% of all, 0.03% of stack)
UpdateStackRef[perm   -> null  ]: 358691495 (3.19% of all, 3.30% of stack)
UpdateStackRef[frozen -> normal]: 14 (0.00% of all, 0.00% of stack)
UpdateStackRef[frozen -> perm  ]: 2805171 (0.02% of all, 0.03% of stack)
UpdateStackRef[frozen -> frozen]: 522735780 (4.65% of all, 4.81% of stack)
UpdateStackRef[frozen -> null  ]: 700877464 (6.23% of all, 6.46% of stack)
UpdateStackRef[null   -> normal]: 3256035093 (28.96% of all, 29.99% of stack)
UpdateStackRef[null   -> perm  ]: 358834098 (3.19% of all, 3.30% of stack)
UpdateStackRef[null   -> frozen]: 700734248 (6.23% of all, 6.45% of stack)
UpdateStackRef[null   -> null  ]: 102765870 (0.91% of all, 0.95% of stack)

SvyatoslavScherbina · 2019-03-22T08:32:02Z

runtime/src/main/cpp/Memory.cpp

@@ -883,23 +883,34 @@ void CollectWhite(MemoryState* state, ContainerHeader* start) {
 }
 #endif

-ALWAYS_INLINE inline void addRef(ContainerHeader* header) {
+inline bool needAtomicAccess(ContainerHeader* container) {
+  return container->shareable() || isAggregatingFrozenContainer(container);


Does || isAggregatingFrozenContainer(container) have any effect?

Yeah, debugging remnant.

olonho and others added 4 commits March 20, 2019 19:46

Explicit stack/heap writes.

96d8a0b

Test runner not exiting the process, to ensure we can check for leaks…

29bab20

… and print stats.

Disable stat collection.

ae5db46

Perf fixes.

29e9fe1

SvyatoslavScherbina reviewed Mar 21, 2019

View reviewed changes

olonho force-pushed the less_stack branch from cbb8983 to 963876b Compare March 22, 2019 05:59

SvyatoslavScherbina reviewed Mar 22, 2019

View reviewed changes

SvyatoslavScherbina approved these changes Mar 22, 2019

View reviewed changes

Review feedback.

c74f67c

olonho force-pushed the less_stack branch from 963876b to c74f67c Compare March 22, 2019 12:49

olonho merged commit c3b2b47 into master Mar 22, 2019

olonho deleted the less_stack branch March 22, 2019 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cosmetic change to distinguish stack/heap writes and collect stats. #2794

Cosmetic change to distinguish stack/heap writes and collect stats. #2794

olonho commented Mar 20, 2019

SvyatoslavScherbina Mar 21, 2019

SvyatoslavScherbina Mar 21, 2019

SvyatoslavScherbina Mar 21, 2019

SvyatoslavScherbina Mar 21, 2019

SvyatoslavScherbina Mar 21, 2019

olonho commented Mar 21, 2019

SvyatoslavScherbina Mar 22, 2019

olonho Mar 22, 2019

Cosmetic change to distinguish stack/heap writes and collect stats. #2794

Cosmetic change to distinguish stack/heap writes and collect stats. #2794

Conversation

olonho commented Mar 20, 2019

SvyatoslavScherbina Mar 21, 2019

Choose a reason for hiding this comment

SvyatoslavScherbina Mar 21, 2019

Choose a reason for hiding this comment

SvyatoslavScherbina Mar 21, 2019

Choose a reason for hiding this comment

SvyatoslavScherbina Mar 21, 2019

Choose a reason for hiding this comment

SvyatoslavScherbina Mar 21, 2019

Choose a reason for hiding this comment

olonho commented Mar 21, 2019

SvyatoslavScherbina Mar 22, 2019

Choose a reason for hiding this comment

olonho Mar 22, 2019

Choose a reason for hiding this comment