alignment-aware memory allocation #1605

chenbd · 2018-01-12T14:58:27Z

kazuho

I think that we are in a very good direction. Thank you for working on the improvement.

I have laid down my concerns below. Please let me know what you think.

kazuho · 2018-01-13T01:00:13Z

include/h2o/memory.h

-void *h2o_mem_alloc_pool(h2o_mem_pool_t *pool, size_t sz);
+void *h2o_mem_alloc_pool_aligned(h2o_mem_pool_t *pool, size_t alignment, size_t size);
+
+#ifdef H2O_NO_BUILT_IN_ALIGNOF


Please test the name of the compiler using #if here to determine if we can use __alignof__, instead of determining it in CMakeFiles.txt.

The header file will be used from other applications and that might not involve the invocation of CMake.

Do not determining it in CMakeFiles.txt sounds good.

i searched on the web and seems most compilers support __alignof__.
if we use #if to judge, i'm confused about the judge condition.
is there any platform we need support but missing __alignof__?

That's a very good question.

I now think that we do not need a check. Let's expect that __alignof__ is always available. We already use things like __builtin_clzll; it is likely that the range of compilers we support will remain the same.

kazuho · 2018-01-13T01:01:15Z

include/h2o/memory.h

+#else
+#define H2O_ALIGNOF(type) (__alignof__(type))
+#endif
+#define H2O_ALIGN(x, a) (((x) + (a)-1) & ~((a)-1))


I would appreciate it if we could refrain from adding a public interface to calculate alignment. My understanding is that it is only used in memory.c.

yes, will be fixed later.

kazuho · 2018-01-13T01:05:01Z

lib/common/memory.c

 {
    void *ret;

+    assert(alignment <= 2 * sizeof(void *) && sz % alignment == 0);


Can we remove this assert?

I do not think that alignment <= 2 * sizeof(void *) is something that is guaranteed on every platform. sz % alignment == 0 is going to be slow, and I do not think we need to test this considering how the function is used; it's essentially trying to detect a compiler bug in using the compiler itself.

Related to this, I think that we should eliminate st_h2o_mem_pool_chunk_t::_dummy. Instead, I would suggest changing the definition of st_h2o_mem_pool_chunk_t to something like below and start h2o_mem_pool_t::chunk_offset from offsetof(union un_h2o_mem_pool_chunk_t, next).

union un_h2o_mem_pool_chunk_t { struct st_h2o_mem_pool_chunk_t *next; char bytes[4096]; };

fixed, please check commit 4dd533d

kazuho · 2018-01-13T01:06:46Z

lib/common/memory.c

-    if (sz == 0)
-        sz = 1;
+    if (H2O_UNLIKELY(sz == 0))
+        sz = alignment;


I think that h2o_mem_alloc_pool(int, 0) should return a unique pointer that is aligned for int. Hence keep the code as-is.

you mean leave sz = 1 as-is?
seems we can do this.

even with sz = alignment, h2o_mem_alloc_pool(int, 0) also return a unique pointer that is aligned for int.

Thank you for the changes.

FTR the issue was that there was a case that left the chunk_offset unchanged leading to the uniqueness not being guaranteed; for example when chunk_offset is a multiple of 4 and h2o_mem_alloc_pool(pool, uint32_t, 0) is called.

kazuho · 2018-01-13T01:21:50Z

lib/common/memory.c

    if (pool != NULL) {
-        new_entries = h2o_mem_alloc_pool(pool, element_size * vector->capacity);
+        new_entries =
+            h2o_mem_alloc_pool_aligned(pool, 2 * sizeof(void *), H2O_ALIGN(element_size * vector->capacity, 2 * sizeof(void *)));


How about passing the alignment requirement as an argument to h2o_vector__expand? Call to the function is wrapped via h2o_vector_reserve in which you can get the name of the typed element.

fixed. please check commit 4dd533d.

kazuho

Thank you for the changes.

I think that only cosmetic issues are left. I have left my comments inline, please let me know what you think.

kazuho · 2018-01-14T05:24:32Z

lib/common/memory.c

 {
    pool->chunks = NULL;
-    pool->chunk_offset = sizeof(pool->chunks->bytes);
+    pool->chunk_offset = sizeof(*pool->chunks);


Would you mind elaborating why the change is necessary? I see that you have changed all the sizeofs related to the calculation of the buffer size.

My understanding is that pool->chunk_offset has been (and can continue to be) a offset for (st|un)_h2o_mem_pool_chunk_t::bytes, and I do not understand why you need to change.

because now un_h2o_mem_pool_chunk_t is a union,

sizeof(*pool->chunks) is equal with sizeof(pool->chunks->bytes) and it seems more simpler.
we can also stay what it is.

Thank you for the answer.

I have switched the code back to use sizeof ...bytes. In addition to being more natural (as stated in my comment above) it does not require the knowledge that 1) bytes is the largest member of a 2) union.

kazuho · 2018-01-14T05:26:06Z

lib/common/memory.c

+        pool->chunk_offset = sizeof(union un_h2o_mem_pool_chunk_t *);
    }

+    pool->chunk_offset = H2O_ALIGN(pool->chunk_offset, alignment);


Do we need to make this calculation before if (sizeof(*pool->chunks) - pool->chunk_offset < sz)?

yes, we need make chunk_offset align before judge, also when new chunk is allocated.

kazuho · 2018-01-14T05:28:51Z

lib/common/memory.c

    void *ret;

-    if (sz >= sizeof(pool->chunks->bytes) / 4) {
+    if (sz >= sizeof(*pool->chunks) / 4) {


Maybe this should be sz >= (sizeof(pool->chunks->bytes) - sizeof(pool->chunks->next)) / 4?

yes.
we can change this if we want exactly the same as before.

kazuho · 2018-01-14T05:30:13Z

lib/common/memory.c

        newp->next = pool->chunks;
        pool->chunks = newp;
-        pool->chunk_offset = 0;
+        pool->chunk_offset = sizeof(union un_h2o_mem_pool_chunk_t *);


Can we change the sizeof to sizeof(newp->next)?

Relying on the variable name is more prone to errors than copy-pasting the type of the variable name.

yes, will be fixed.

alignment-aware memory allocation

kazuho · 2018-01-15T14:35:45Z

Thank you for your work on the issue! I have merged the pull request after making some cosmetic changes. I think that all the changes are fine, but please let me know if you have any concern.

try adding aligment to "h2o_mem_alloc_pool()"

adc3b6b

issues h2o#1597

kazuho reviewed Jan 13, 2018

View reviewed changes

fix review issues for "h2o_mem_alloc_pool()"

4dd533d

kazuho reviewed Jan 14, 2018

View reviewed changes

fix review issues for "h2o_mem_alloc_pool()"

a860828

kazuho changed the title ~~try adding aligment to "h2o_mem_alloc_pool()"~~ alignment-aware memory allocation Jan 15, 2018

kazuho merged commit a860828 into h2o:master Jan 15, 2018

kazuho added a commit that referenced this pull request Jan 15, 2018

Merge pull request #1605

ca432ef

alignment-aware memory allocation

alignment-aware memory allocation #1605

alignment-aware memory allocation #1605

Uh oh!

Conversation

chenbd commented Jan 12, 2018

Uh oh!

kazuho left a comment

Choose a reason for hiding this comment

Uh oh!

kazuho Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenbd Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenbd Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazuho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenbd Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazuho commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kazuho Jan 13, 2018 •

edited

Loading

chenbd Jan 13, 2018 •

edited

Loading

chenbd Jan 13, 2018 •

edited

Loading

chenbd Jan 14, 2018 •

edited

Loading