Allocate objects in pools with correct alignment #21959

vchuravy · 2017-05-19T10:09:01Z

Fixes #21918.

The issue was that out gc pools only guaranteed 16 bytes alignment and the pool pages are only aligned by 16 bytes as well.
This PR changes the alignment of the pool pages to 64 bytes (enough for avx512) and takes the alignment information into account when selecting the szclass/pool.
It correctly handles unaligned allocations and allocations that are near the maximum pool size.

This should be squashed a bit before we merge.

yuyichao · 2017-05-19T12:45:33Z

src/julia_internal.h


 STATIC_INLINE jl_value_t *jl_gc_alloc_(jl_ptls_t ptls, size_t sz, void *ty)
 {
    const size_t allocsz = sz + sizeof(jl_taggedvalue_t);
    if (allocsz < sz) // overflow in adding offs, size was "negative"
        jl_throw(jl_memory_exception);
+    size_t alignment = JL_SMALL_BYTE_ALIGNMENT;
+    if (ty && ((uintptr_t)ty != jl_buff_tag) &&


This function shouldn't read ty.

Why? I need access to the alignment at this point and ty is used in jl_set_typeof(v, ty) later in the function.
Otherwise I will need to pass in the alignment from the outside.

The size is also passed in explicitly. I think we can just have an aligned version

yuyichao · 2017-05-19T12:46:52Z

base/atomics.jl

@@ -322,7 +322,7 @@ inttype(::Type{Float32}) = Int32
 inttype(::Type{Float64}) = Int64


-alignment(::Type{T}) where {T} = ccall(:jl_alignment, Cint, (Csize_t,), sizeof(T))
+alignment(::Type{T}) where {T} = ccall(:jl_alignment, Cint, (Any,), T)


This can be removed.

(it's just datatype_alignment now.)

yuyichao · 2017-05-19T12:53:36Z

src/julia_internal.h

@@ -170,30 +176,25 @@ static const int jl_gc_sizeclasses[JL_GC_N_POOLS] = {
 //    64,   32,  160,   64,   16,   64,  112,  128, bytes lost


And we should change this list for this.

tkelman · 2017-05-19T13:07:42Z

src/julia_internal.h

-    // szclass 16+
-    return 16;
-#endif
+    // The pools are aligned wit JL_CACHE_BYTE_ALIGNMENT (typically 64)


tkelman · 2017-06-01T05:45:51Z

src/julia_internal.h

+#define JL_CACHE_BYTE_ALIGNMENT 64
+// JL_HEAP_ALIGNMENT is the maximum alignment that the GC can provide
+#define JL_HEAP_ALIGNMENT JL_SMALL_BYTE_ALIGNMENT
+#define GC_MAX_SZCLASS (2032-sizeof(void*))


what's the significance of the 2032?

It is the largest object size that we use pools for. Also, see the last entry in jl_gc_sizeclasses

what are the chances of it changing in either place?

I'm not sure what the sizeof(void*) comes from, but this PR is supposed to be changing this list (#21959 (comment))

vchuravy · 2017-05-25T05:32:22Z

src/ccall.cpp

@@ -1214,8 +1214,9 @@ static jl_cgval_t mark_or_box_ccall_result(Value *result, bool isboxed, jl_value
        const DataLayout &DL = *jl_ExecutionEngine->getDataLayout();
 #endif
        unsigned nb = DL.getTypeStoreSize(result->getType());
+        unsigned alignment = DL.getPrefTypeAlignment(result->getType());


This could also be DL.getABITypeAlignment(result->getType())

vchuravy · 2017-06-01T05:52:37Z

src/julia_internal.h

+#define JL_CACHE_BYTE_ALIGNMENT 64
+// JL_HEAP_ALIGNMENT is the maximum alignment that the GC can provide
+#define JL_HEAP_ALIGNMENT JL_SMALL_BYTE_ALIGNMENT
+#define GC_MAX_SZCLASS (2032-sizeof(void*))


It is the largest object size that we use pools for. Also, see the last entry in jl_gc_sizeclasses

vtjnash · 2017-06-01T06:57:46Z

src/julia_internal.h

-        return 16 - 16376 / 2 / LLT_ALIGN(sz, 16 * 2) + 24 + N;
-    return     16 - 16376 / 1 / LLT_ALIGN(sz, 16 * 1) + 32 + N;
+    size_t klass = 0;
+    while (klass < JL_GC_N_POOLS) {


this seems way more expensive (also given that this function is already a pretty significant percentage of the time to allocate an object)

Yeah I know I wanted a fast way to check correctness before going back to the jump table.

vtjnash · 2017-06-01T06:59:43Z

src/julia_internal.h

+#define JL_CACHE_BYTE_ALIGNMENT 64
+// JL_HEAP_ALIGNMENT is the maximum alignment that the GC can provide
+#define JL_HEAP_ALIGNMENT JL_SMALL_BYTE_ALIGNMENT
+#define GC_MAX_SZCLASS (2032-sizeof(void*))


I'm not sure what the sizeof(void*) comes from, but this PR is supposed to be changing this list (#21959 (comment))

vchuravy · 2017-06-02T06:09:41Z

@jlbuild !filter=linux

jlbuild · 2017-06-02T06:09:49Z

Status of `f487f73` builds:

Builder Name	Build	Download
linux32	COMPLETE	Download
linux64	COMPLETE	Download
linuxaarch64	ERRORED	N/A
linuxarmv7l	COMPLETE	Download
linuxppc64le	COMPLETE	Download

vchuravy · 2017-06-02T06:25:26Z

AppVeyor failure is #20152 (comment) ...

vtjnash · 2017-07-15T02:00:14Z

Why did this stall out? Are we close in it?

vchuravy · 2017-07-15T02:22:20Z

I got sidetracked due to my relocation and I won't have time until early August to continue working on this. As far as I know the missing piece are: The pool sizes need to be optimised and the function that selects the correct pool under alignment consideration needs to be rewritten and optimised. If anybody would like to take a stab feel free to do so!

…

On Sat, 15 Jul 2017, 11:00 Jameson Nash, ***@***.***> wrote: Why did this stall out? Are we close in it? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21959 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI3atoXS91693ibQdYyOaxu8qys0bdBks5sOB0vgaJpZM4NgVRE> .

JeffBezanson · 2017-09-24T16:41:25Z

Bump. This is becoming a bit urgent, as we have frequent CI failures due to this issue.

vchuravy · 2017-09-24T21:57:04Z

OK, I rebased this so it is on the same stand as earlier this year. I will need help with making sure that the logic for jl_gc_szclass is sound and performant.

-- edit I hope I got the rebase right and complete, so a review would be appreciated

vchuravy · 2017-09-25T04:39:45Z

freebsd CI is a wonderful LLVM ERROR: Program used external function '__atomic_store' which could not be resolved!. This means probably two things.

@ararslan & @iblis17 are we using libatomic on FreeBSD?
The alignments for one of the atomic types is off, and we are outputting a generic atomic_store

AppVeyor is a lovely:

From worker 16:	Warning: threaded loop executed in order
	From worker 16:	
	From worker 16:	Please submit a bug report with steps to reproduce this fault, and any error messages that follow (in their entirety). Thanks.
	From worker 16:	Exception: EXCEPTION_ACCESS_VIOLATION at 0x6c6c15c0 -- _atomic_load at C:\projects\julia\julia-f569b7bf15\bin\libatomic-1.DLL (unknown line)
	From worker 16:	in expression starting at C:\projects\julia\julia-f569b7bf15\share\julia\test\threads.jl:271
	From worker 16:	_atomic_load at C:\projects\julia\julia-f

Which shouldn't have seqfaulted, but is probably a wrong alignment as well.

and CircleCI seems unrelated.

yuyichao · 2017-09-25T04:41:33Z

are we using libatomic on FreeBSD?

That code is not turned on for FreeBSD.

vchuravy · 2017-09-25T11:49:38Z

One new test failure from Travis Mac 64bit

	From worker 8:	union_isbits at /Users/travis/build/JuliaLang/julia/src/datatype.c:244
	From worker 8:	union_isbits at /Users/travis/build/JuliaLang/julia/src/datatype.c:248
	From worker 8:	jl_islayout_inline at /Users/travis/build/JuliaLang/julia/src/datatype.c:267
	From worker 8:	_new_array at /Users/travis/build/JuliaLang/julia/src/array.c:146 [inlined]
	From worker 8:	jl_alloc_array_1d at /Users/travis/build/JuliaLang/julia/src/array.c:415

and #23371 on Travis Linux 32bit.

vchuravy · 2017-09-25T14:02:34Z

As far as I am aware one wants to align atomic types by their natural alignment e.g. sizeof == alignment.

On current master this invariant in correct:

T = Base.Threads.Atomic{Int128}
julia> Base.Threads.alignment(T)
16
julia> Base.Threads.alignment(T) == Base.sizeof(T)
true
julia> Base.datatype_alignment(T)
8
julia> Base.datatype_alignment(Int128)
8

On this branch there is no longer a special alignment query function since the ideas was to only have one source of truths for all alignments and as one can see already on master datatype_alignment and Threads.alignment disagree for (U)Int128, which turns out to be related to #22649 (a classic case of mobius #yak shaving)

vtjnash · 2017-09-25T15:45:23Z

src/julia.h

@@ -155,6 +155,7 @@ JL_EXTENSION typedef struct {
 #endif
    jl_array_flags_t flags;
    uint16_t elsize;
+    uint16_t elalign;


why is this needed? we don't have any spare bits right here, but you can steal some from offset

It is used in multiple places like jl_array_copy and during deserialization to create a new array with the right alignment.

Sure, but those aren't performance sensitive. How does this differ from
el->isptr ? sizeof(void*) ? jl_gc_align(jl_tparam0(jl_typeof(el))?

Sure that should work, I was mostly mirroring what elsize is doing.

vtjnash · 2017-09-25T15:52:24Z

src/julia_internal.h

+#define JL_CACHE_BYTE_ALIGNMENT 64
+// JL_HEAP_ALIGNMENT is the maximum alignment that the GC can provide
+#define JL_HEAP_ALIGNMENT JL_CACHE_BYTE_ALIGNMENT
+#define GC_MAX_SZCLASS (2032-sizeof(void*))


2032 has no significance. It is simply:
(((GC_PAGE_SZ - ALIGN + sizeof(jl_taggedvalue_t)) ÷ 8) ÷ ALIGN) * ALIGN
per the formula below (the 8 is arbitrary)

vtjnash · 2017-09-25T16:04:22Z

src/julia_internal.h

+    // An alignment of 0 or 1 means unaligned and we can use sz directly.
+    if (alignment != 0 && alignment != 1 && alignment != sz)
+        sz = ((sz / alignment) + 1) * alignment;
+    return sz;


size_t alsz = LLT_ALIGN(sz, alignment); return alignment ? alsz : sz;

vtjnash · 2017-09-25T16:08:14Z

src/julia_internal.h

    jl_value_t *v;
-    if (allocsz <= GC_MAX_SZCLASS + sizeof(jl_taggedvalue_t)) {
-        int pool_id = jl_gc_szclass(allocsz);
+    if (klass != -1 && alignsz <= GC_MAX_SZCLASS + sizeof(jl_taggedvalue_t)) {


size test seems unnecessary (redundant with klass test)

vtjnash · 2017-09-25T16:19:11Z

src/julia_internal.h

            osize = jl_gc_sizeclasses[pool_id];
        }
        else {
            osize = p->osize;
        }
+        assert((size_t)osize >= alignment &&
+               (alignment == 0 || alignment == 1 || osize % alignment == 0));


alignment == 0 || (osize & (alignment - 1)) == 0 (if you want, also can add && (alignment & (alignment - 1) == 0) to assert that alignment is a power-of-two)

vtjnash · 2017-09-25T16:20:27Z

src/julia_internal.h

 #else
-#  define jl_gc_alloc(ptls, sz, ty) jl_gc_alloc_(ptls, sz, ty)
+#  define jl_gc_alloc(ptls, sz, align, ty) jl_gc_alloc_(ptls, sz, align, ty)


should keep the same names on both sides of the conditional

vchuravy · 2017-09-25T17:38:09Z

test/threads.jl

@@ -280,6 +280,10 @@ let atomic_types = [Int8, Int16, Int32, Int64, Int128,
        filter!(T -> sizeof(T)<=8, atomic_types)
    end
    for T in atomic_types
+	# Check that alignment is natural alignment
+	@test Base.datatype_alignment(T) == Base.sizeof(T)


This test is currently broken for Atomic{(U)Int128}

vchuravy · 2017-09-25T18:44:53Z

src/dump.c

@@ -1349,7 +1355,7 @@ static jl_value_t *jl_deserialize_value_array(jl_serializer_state *s, jl_value_t
    for (i = 0; i < ndims; i++) {
        dims[i] = jl_unbox_long(jl_deserialize_value(s, NULL));
    }
-    jl_array_t *a = jl_new_array_for_deserialization((jl_value_t*)NULL, ndims, dims, isunboxed, elsize);
+    jl_array_t *a = jl_new_array_for_deserialization((jl_value_t*)NULL, ndims, dims, isunboxed, elsize, elalign);


@vtjnash Any idea how to get elalign at this point of the serialisation state? I don't have a eltype here yet, right? For that I need aty and that is depended on a

compute it during serialization

Ok just not keep it around ;) Gotcha

ararslan · 2017-09-25T19:35:16Z

src/julia_internal.h

-    if (allocsz <= GC_MAX_SZCLASS + sizeof(jl_taggedvalue_t)) {
-        int pool_id = jl_gc_szclass(allocsz);
+    if (klass != -1){
+	assert(alignsz <= GC_MAX_SZCLASS + sizeof(jl_taggedvalue_t));


Wrong indentation here (should be two four-space indents rather than a tab)

ararslan · 2017-09-25T19:36:09Z

src/llvm-alloc-opt.cpp

            continue;
-        it.first->eraseFromParent();
+	std::get<0>(it)->eraseFromParent();


Wrong indentation here too

ararslan · 2017-09-25T19:36:46Z

test/threads.jl

@@ -280,6 +280,14 @@ let atomic_types = [Int8, Int16, Int32, Int64, Int128,
        filter!(T -> sizeof(T)<=8, atomic_types)
    end
    for T in atomic_types
+	# Check that alignment is natural alignment


These lines too

vchuravy · 2017-09-27T14:57:53Z

OK todos after the recent rebase:

address Jameson comment and use jl_islayout_inline
cleanup the mess that is jl_alignment,julia_alignment, datype_alignment, and jl_gc_alignment
optimise the pool osize table and selection
stress test this

After my discussion with Jameson yesterday I am worried about the difference between stack-alignment and heap-alignment. Currently they both agree at 16, but part of the reason for this whole PR is that I would like to bump heap alignment to 64 so that we can support alignments of 32 and 64 (avx256, avx512) Do we also need to bump the stack alignment?

yuyichao · 2017-10-05T22:26:41Z

src/llvm-alloc-opt.cpp

        if (!ignore_tag) {
-            align = sz <= 8 ? 8 : JL_SMALL_BYTE_ALIGNMENT;
            sz += align;


This should change alignment to pointer size and increase size by that much.

JeffreySarnoff · 2018-05-03T02:26:24Z

The memory alignment used by Julia should be 64. No good comes from bifurcating the manner of [co]locating information. Stack and heap should be transparently egalitarian in their conferred alignment. This provides palpable future-proofing and levers performance (which follows less mess).

* TODO: select bucket that fits multiple of alignment * TODO: allow alignment up to 64 * TODO: make arrays follow alignment as well * TODO: cleanup the mess that is jl_alignment, julia_alignment, datype_alignment, and jl_gc_alignment * TODO: teach jl_gc_alloc_ to do something with the alignment request

vtjnash · 2021-05-05T17:54:53Z

Discussing with @vchuravy today, the last missing bit is computing the object padding required to provide the API here. It is a slight change to the way this works now, but should independently help finish #22649 too.

vchuravy · 2023-04-26T19:37:05Z

Very stale

vtjnash · 2023-04-27T02:43:40Z

I thought we had almost finished this. Just needed a small rebase

vchuravy · 2023-04-27T02:53:02Z

Just needed a small rebase

Just doing a lot of heavy lifting here. :)
I won't be working on this anytime soon, so in some sense I am unlicking the cookie.

vchuravy added backport pending 0.6 bugfix This change fixes an existing bug GC Garbage collector labels May 19, 2017

vchuravy requested a review from yuyichao May 19, 2017 10:09

yuyichao requested changes May 19, 2017

View reviewed changes

tkelman reviewed May 19, 2017

View reviewed changes

vchuravy force-pushed the vc/pool_alignment branch 2 times, most recently from 19b847c to ab60fc6 Compare May 20, 2017 06:03

vchuravy mentioned this pull request May 20, 2017

limit maximum vector alignment to heap alignment #21980

Merged

vchuravy removed the backport pending 0.6 label May 22, 2017

vchuravy force-pushed the vc/pool_alignment branch 2 times, most recently from 19ee5f9 to dcf3920 Compare May 25, 2017 05:30

vchuravy force-pushed the vc/pool_alignment branch from 7e38329 to fe5bd9f Compare June 1, 2017 05:36

tkelman reviewed Jun 1, 2017

View reviewed changes

vchuravy commented Jun 1, 2017

View reviewed changes

vtjnash reviewed Jun 1, 2017

View reviewed changes

vchuravy force-pushed the vc/pool_alignment branch 2 times, most recently from 33b89c5 to 06ed45f Compare June 1, 2017 08:36

vchuravy mentioned this pull request Jun 2, 2017

Install and use libatomic on windows. #22189

Merged

vchuravy mentioned this pull request Sep 20, 2017

crash in vecelement test #23796

Closed

vchuravy force-pushed the vc/pool_alignment branch from f487f73 to 317561d Compare September 24, 2017 21:55

vchuravy force-pushed the vc/pool_alignment branch from 317561d to c4344a2 Compare September 25, 2017 02:02

vtjnash reviewed Sep 25, 2017

View reviewed changes

vchuravy commented Sep 25, 2017

View reviewed changes

ararslan reviewed Sep 25, 2017

View reviewed changes

vchuravy force-pushed the vc/pool_alignment branch from 11d181f to 4d0dc1e Compare September 27, 2017 14:58

yuyichao reviewed Oct 5, 2017

View reviewed changes

vchuravy mentioned this pull request Mar 6, 2018

Issues on Julia 0.6 and 0.7 eschnett/SIMD.jl#27

Closed

vtjnash self-assigned this Mar 9, 2018

vtjnash mentioned this pull request Sep 18, 2018

faster String allocation #29254

Merged

vtjnash mentioned this pull request Dec 18, 2018

Segmentation faults with NTuple{2,NTuple{N,Core.VecElement{T}}}, but not NTuple{2N,Core.VecElement{T}} #30426

Closed

simonbyrne mentioned this pull request Feb 18, 2019

Float128 calling convention and alignment JuliaMath/Quadmath.jl#4

Closed

vtjnash mentioned this pull request Jun 4, 2019

change array realloc alignment to 16 bytes? #32236

Closed

vtjnash mentioned this pull request Jun 20, 2019

armv7l SIMD errors #24728

Open

vtjnash mentioned this pull request Oct 19, 2019

Wrong GEP emission for tuple containing VecElement #32414

Closed

vtjnash force-pushed the vc/pool_alignment branch from 4d0dc1e to 0deb205 Compare January 22, 2020 21:38

vtjnash force-pushed the vc/pool_alignment branch from 0deb205 to f1586ad Compare October 30, 2020 17:23

gbaraldi mentioned this pull request Jan 5, 2023

Make sure LLVM knows our allocation alignment #48139

Open

vchuravy closed this Apr 26, 2023

vchuravy mentioned this pull request Mar 11, 2024

vecelement test fails on Zen4 #53683

Closed

giordano deleted the vc/pool_alignment branch March 12, 2024 00:19

		@@ -170,30 +176,25 @@ static const int jl_gc_sizeclasses[JL_GC_N_POOLS] = {
		// 64, 32, 160, 64, 16, 64, 112, 128, bytes lost

Allocate objects in pools with correct alignment #21959

Allocate objects in pools with correct alignment #21959

Conversation

vchuravy commented May 19, 2017

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

vchuravy Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchuravy commented Jun 2, 2017

jlbuild commented Jun 2, 2017 • edited Loading

Status of f487f73 builds:

vchuravy commented Jun 2, 2017

vtjnash commented Jul 15, 2017

vchuravy commented Jul 15, 2017 via email

JeffBezanson commented Sep 24, 2017

vchuravy commented Sep 24, 2017 • edited Loading

vchuravy commented Sep 25, 2017

yuyichao commented Sep 25, 2017

vchuravy commented Sep 25, 2017 • edited Loading

vchuravy commented Sep 25, 2017

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

vchuravy commented Sep 27, 2017 • edited by vtjnash Loading

Choose a reason for hiding this comment

JeffreySarnoff commented May 3, 2018

vtjnash commented May 5, 2021

vchuravy commented Apr 26, 2023

vtjnash commented Apr 27, 2023

vchuravy commented Apr 27, 2023

vchuravy Jun 1, 2017 •

edited

Loading

jlbuild commented Jun 2, 2017 •

edited

Loading

Status of `f487f73` builds:

vchuravy commented Sep 24, 2017 •

edited

Loading

vchuravy commented Sep 25, 2017 •

edited

Loading

vchuravy commented Sep 27, 2017 •

edited by vtjnash

Loading