Use sizeof more pervasively #242

compnerd · 2017-04-24T21:01:27Z

Change a few places to use sizeof rather than hard coding the size of structures and types based on pointer size.

compnerd · 2017-04-24T21:07:58Z

MadCoder

you can't use that compiler specific sizeof pointer

MadCoder · 2017-04-24T21:09:49Z

src/queue_internal.h

 #endif
 #define _DISPATCH_CONTINUATION_PTRS 8
 #if DISPATCH_HW_CONFIG_UP
 // UP devices don't contend on continuations so we don't need to force them to
 // occupy a whole cacheline (which is intended to avoid contention)
 #define DISPATCH_CONTINUATION_SIZE \
-		(_DISPATCH_CONTINUATION_PTRS * _DISPATCH_SIZEOF_PTR)
+		(_DISPATCH_CONTINUATION_PTRS * __SIZEOF_POINTER__)


__SIZEOF_POINTER__ is not standard afaict which is why dispatch does it this way.

Right, it's not standard. Isn't clang a requisite though? The other ones seem fine though, so, let me drop this part, and we can address it separately.

BTW, is __LP64__ standard? I don't believe that MSVC defines that.

compnerd · 2017-04-24T21:47:18Z

Dropped the __SIZEOF_POINTER__ changes, which we can discuss separately.

compnerd · 2017-04-25T00:24:40Z

@MadCoder anything else for this part of the change?

MadCoder · 2017-04-25T00:30:01Z

src/allocator_internal.h

-#else
-#define SIZEOF_HEADER 8
-#endif
-


I'd rather keep the SIZEOF_HEADER to a define to the right sizeof()

MadCoder · 2017-04-25T00:30:18Z

src/allocator_internal.h


 #define MAPS_TO_FPMAPS_PADDING (PADDING_TO_CONTINUATION_SIZE(SIZEOF_MAPS))

 #define BYTES_LEFT_IN_FIRST_PAGE (BYTES_PER_PAGE - \
-		(SIZEOF_HEADER + HEADER_TO_SUPERMAPS_PADDING + SIZEOF_SUPERMAPS + \
+		(sizeof(struct dispatch_magazine_header_s) + HEADER_TO_SUPERMAPS_PADDING + SIZEOF_SUPERMAPS + \


and keep SIZEOF_HEADER here which has the side effect of keeping it to 80 columns

MadCoder · 2017-04-25T00:30:51Z

src/allocator_internal.h

@@ -97,25 +97,20 @@
 // Use the largest type your platform is comfortable doing atomic ops with.
 // TODO: rdar://11477843
 typedef unsigned long bitmap_t;
-#if defined(__LP64__)
-#define BYTES_PER_BITMAP 8


same as the other guy, makes this sizeof(bitmap_t)

MadCoder · 2017-04-25T00:31:05Z

src/allocator_internal.h


 #define BITMAP_C(v) ((bitmap_t)(v))
 #define BITMAP_ALL_ONES (~BITMAP_C(0))

 // Stop configuring.

-#define CONTINUATIONS_PER_BITMAP (BYTES_PER_BITMAP * 8)
+#define CONTINUATIONS_PER_BITMAP (sizeof(bitmap_t) * 8)


if you're feeling pedantic, that 8 should be CHAR_BIT

MadCoder · 2017-04-25T00:31:16Z

src/allocator_internal.h

@@ -138,8 +133,8 @@ typedef unsigned long bitmap_t;
 // this will round up such that first_bitmap_in_same_page() can mask the address
 // of a bitmap_t in the maps to obtain the first bitmap for that same page
 #define ROUND_UP_TO_BITMAP_ALIGNMENT(x) \
-		(((x) + ((BITMAPS_PER_PAGE * BYTES_PER_BITMAP) - 1u)) & \
-		~((BITMAPS_PER_PAGE * BYTES_PER_BITMAP) - 1u))


MadCoder · 2017-04-25T00:31:24Z

src/allocator_internal.h

@@ -148,7 +143,7 @@ typedef unsigned long bitmap_t;
 #define PADDING_TO_CONTINUATION_SIZE(x) (ROUND_UP_TO_CONTINUATION_SIZE(x) - (x))

 #define SIZEOF_SUPERMAPS (BYTES_PER_SUPERMAP * SUPERMAPS_PER_MAGAZINE)
-#define SIZEOF_MAPS (BYTES_PER_BITMAP * BITMAPS_PER_SUPERMAP * \
+#define SIZEOF_MAPS (sizeof(bitmap_t) * BITMAPS_PER_SUPERMAP * \


MadCoder · 2017-04-25T00:31:27Z

src/allocator_internal.h

@@ -171,7 +166,7 @@ typedef unsigned long bitmap_t;
 #define REMAINDER_IN_FIRST_PAGE (BYTES_LEFT_IN_FIRST_PAGE - \
 		(FULL_BITMAPS_IN_FIRST_PAGE * CONSUMED_BYTES_PER_BITMAP) - \
 		(FULL_BITMAPS_IN_FIRST_PAGE ? 0 : \
-		ROUND_UP_TO_CONTINUATION_SIZE(BYTES_PER_BITMAP)))


MadCoder · 2017-04-25T00:31:35Z

src/allocator_internal.h

@@ -181,7 +176,7 @@ typedef unsigned long bitmap_t;
 		(REMAINDERED_CONTINUATIONS_IN_FIRST_PAGE == 0 ? 0 : 1))

 #define FPMAPS_TO_FPCONTS_PADDING (PADDING_TO_CONTINUATION_SIZE(\
-		BYTES_PER_BITMAP * BITMAPS_IN_FIRST_PAGE))


MadCoder · 2017-04-25T00:31:58Z

src/allocator_internal.h

@@ -162,12 +156,12 @@ typedef unsigned long bitmap_t;
 // we want to align the maps to a continuation size, but we must also have proper padding
 // so that we can perform first_bitmap_in_same_page()
 #define SUPERMAPS_TO_MAPS_PADDING (PADDING_TO_BITMAP_ALIGNMENT_AND_CONTINUATION_SIZE( \
-		SIZEOF_SUPERMAPS + HEADER_TO_SUPERMAPS_PADDING + SIZEOF_HEADER))
+		SIZEOF_SUPERMAPS + HEADER_TO_SUPERMAPS_PADDING + sizeof(struct dispatch_magazine_header_s)))


Rather than trying to compute the size of the magazine header manually, use the compiler's builtin `sizeof` operator.

Rather than hardcoding the sizeof(bitmap_t) based on `__LP64__` use the compiler builtin `sizeof` to compute the size.

compnerd · 2017-04-25T15:28:23Z

Updated and also changed the 8 to CHAR_BIT.

MadCoder · 2017-04-26T03:21:44Z

@swift-ci please test

MadCoder · 2017-04-26T15:58:45Z

@swift-ci please test

compnerd · 2017-04-27T18:49:06Z

@MadCoder good to go on this?

das · 2017-05-19T03:26:29Z

I'm going to revert this, it breaks the build on Darwin with:

In file included from libdispatch/src/allocator.c:22:
libdispatch/src/allocator_internal.h:240:5: error: function-like macro 'sizeof' is not defined
#if SUPERMAPS_TO_MAPS_PADDING > 0
    ^
libdispatch/src/allocator_internal.h:168:3: note: expanded from macro 'SUPERMAPS_TO_MAPS_PADDING'
                SIZEOF_SUPERMAPS + HEADER_TO_SUPERMAPS_PADDING + SIZEOF_HEADER))
                ^
libdispatch/src/allocator_internal.h:159:27: note: expanded from macro 'SIZEOF_SUPERMAPS'
#define SIZEOF_SUPERMAPS (BYTES_PER_SUPERMAP * SUPERMAPS_PER_MAGAZINE)
                          ^
libdispatch/src/allocator_internal.h:125:28: note: expanded from macro 'BYTES_PER_SUPERMAP'
#define BYTES_PER_SUPERMAP sizeof(bitmap_t)
                           ^
libdispatch/src/allocator_internal.h:248:5: error: function-like macro 'sizeof' is not defined
#if MAPS_TO_FPMAPS_PADDING > 0
    ^
libdispatch/src/allocator_internal.h:170:62: note: expanded from macro 'MAPS_TO_FPMAPS_PADDING'
#define MAPS_TO_FPMAPS_PADDING (PADDING_TO_CONTINUATION_SIZE(SIZEOF_MAPS))
                                                             ^
libdispatch/src/allocator_internal.h:160:22: note: expanded from macro 'SIZEOF_MAPS'
#define SIZEOF_MAPS (BYTES_PER_BITMAP * BITMAPS_PER_SUPERMAP * \
                     ^
libdispatch/src/allocator_internal.h:111:26: note: expanded from macro 'BYTES_PER_BITMAP'
#define BYTES_PER_BITMAP sizeof(bitmap_t)
                         ^
libdispatch/src/allocator_internal.h:258:5: error: function-like macro 'sizeof' is not defined
#if FPMAPS_TO_FPCONTS_PADDING > 0
    ^
libdispatch/src/allocator_internal.h:193:3: note: expanded from macro 'FPMAPS_TO_FPCONTS_PADDING'
                BYTES_PER_BITMAP * BITMAPS_IN_FIRST_PAGE))
                ^
libdispatch/src/allocator_internal.h:111:26: note: expanded from macro 'BYTES_PER_BITMAP'
#define BYTES_PER_BITMAP sizeof(bitmap_t)
                         ^
libdispatch/src/allocator_internal.h:278:5: error: function-like macro 'sizeof' is not defined
#if AFTER_CONTS_PADDING > 0
    ^
libdispatch/src/allocator_internal.h:202:33: note: expanded from macro 'AFTER_CONTS_PADDING'
                (DISPATCH_CONTINUATION_SIZE * CONTINUATIONS_PER_MAGAZINE)))
                                              ^
libdispatch/src/allocator_internal.h:140:4: note: expanded from macro 'CONTINUATIONS_PER_MAGAZINE'
                (BITMAPS_PER_MAGAZINE * CONTINUATIONS_PER_BITMAP)
                 ^
libdispatch/src/allocator_internal.h:138:31: note: expanded from macro 'BITMAPS_PER_MAGAZINE'
#define BITMAPS_PER_MAGAZINE (SUPERMAPS_PER_MAGAZINE * BITMAPS_PER_SUPERMAP)
                              ^
libdispatch/src/allocator_internal.h:137:3: note: expanded from macro 'SUPERMAPS_PER_MAGAZINE'
                CONSUMED_BYTES_PER_SUPERMAP)
                ^
libdispatch/src/allocator_internal.h:126:38: note: expanded from macro 'CONSUMED_BYTES_PER_SUPERMAP'
#define CONSUMED_BYTES_PER_SUPERMAP (BYTES_PER_SUPERMAP + \
                                     ^
libdispatch/src/allocator_internal.h:125:28: note: expanded from macro 'BYTES_PER_SUPERMAP'
#define BYTES_PER_SUPERMAP sizeof(bitmap_t)
                           ^

MadCoder suggested changes Apr 24, 2017

View reviewed changes

compnerd force-pushed the sizeof branch from c87bd22 to 516f196 Compare April 24, 2017 21:46

MadCoder suggested changes Apr 25, 2017

View reviewed changes

compnerd added 3 commits April 25, 2017 08:23

allocator: use the compiler to compute header size

a697fe7

Rather than trying to compute the size of the magazine header manually, use the compiler's builtin `sizeof` operator.

allocator: use compiler to compute sizeof bitmap_t

0f6e863

Rather than hardcoding the sizeof(bitmap_t) based on `__LP64__` use the compiler builtin `sizeof` to compute the size.

allocator: use CHAR_BIT instead of 8

7f2576c

compnerd force-pushed the sizeof branch from 516f196 to 7f2576c Compare April 25, 2017 15:28

MadCoder approved these changes Apr 26, 2017

View reviewed changes

MadCoder merged commit 14d3d90 into swiftlang:master Apr 27, 2017

MadCoder added the darwin pending label Apr 27, 2017

compnerd deleted the sizeof branch April 28, 2017 22:27

das mentioned this pull request May 19, 2017

Revert "Use sizeof more pervasively" #247

Merged

das removed the darwin pending label May 19, 2017

Use sizeof more pervasively #242

Use sizeof more pervasively #242

Uh oh!

Conversation

compnerd commented Apr 24, 2017

Uh oh!

compnerd commented Apr 24, 2017

Uh oh!

MadCoder left a comment

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!

compnerd commented Apr 24, 2017

Uh oh!

compnerd commented Apr 25, 2017

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!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Apr 25, 2017

Uh oh!

MadCoder commented Apr 26, 2017

Uh oh!

MadCoder commented Apr 26, 2017

Uh oh!

compnerd commented Apr 27, 2017

Uh oh!

das commented May 19, 2017

Uh oh!

Uh oh!