Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let struct dump_config in objspace fit in a single cache line #2274

Closed

Conversation

methodmissing
Copy link
Contributor

Another small packed struct change - gets passed around quite a bit in the dump_append* helper methods

Before:

lourens@CarbonX1:~/src/ruby/trunk$ pahole -C dump_config -P ./.ext/x86_64-linux/objspace.so
struct dump_config {
	VALUE                      type;                 /*     0     8 */
	FILE *                     stream;               /*     8     8 */
	VALUE                      string;               /*    16     8 */
	int                        roots;                /*    24     4 */

	/* XXX 4 bytes hole, try to pack */

	const char  *              root_category;        /*    32     8 */
	VALUE                      cur_obj;              /*    40     8 */
	VALUE                      cur_obj_klass;        /*    48     8 */
	size_t                     cur_obj_references;   /*    56     8 */
	/* --- cacheline 1 boundary (64 bytes) --- */
	int                        full_heap;            /*    64     4 */

	/* size: 72, cachelines: 2, members: 9 */
	/* sum members: 64, holes: 1, sum holes: 4 */
	/* padding: 4 */
	/* last cacheline: 8 bytes */
};

After:

lourens@CarbonX1:~/src/ruby/ruby$ pahole -C dump_config -P ./.ext/x86_64-linux/objspace.so
struct dump_config {
	VALUE                      type;                 /*     0     8 */
	FILE *                     stream;               /*     8     8 */
	VALUE                      string;               /*    16     8 */
	int                        roots;                /*    24     4 */
	int                        full_heap;            /*    28     4 */
	const char  *              root_category;        /*    32     8 */
	VALUE                      cur_obj;              /*    40     8 */
	VALUE                      cur_obj_klass;        /*    48     8 */
	size_t                     cur_obj_references;   /*    56     8 */

	/* size: 64, cachelines: 1, members: 9 */
};

@nobu
Copy link
Member

nobu commented Jul 4, 2019

They are used as just booleans, and could be bit flags.

@methodmissing
Copy link
Contributor Author

True, good point - I can change it a bit later today to bit flags.

@methodmissing
Copy link
Contributor Author

methodmissing commented Jul 4, 2019

With bit flags for the booleans in a0e50c3 - introduced the flags member to namespace bit flags as per convention in gc.c and other source files :

struct dump_config {
	VALUE                      type;                 /*     0     8 */
	FILE *                     stream;               /*     8     8 */
	VALUE                      string;               /*    16     8 */
	const char  *              root_category;        /*    24     8 */
	VALUE                      cur_obj;              /*    32     8 */
	VALUE                      cur_obj_klass;        /*    40     8 */
	size_t                     cur_obj_references;   /*    48     8 */
	struct {
		unsigned int       roots:1;              /*    56: 0  4 */
		unsigned int       full_heap:1;          /*    56: 1  4 */
	} flags;                                         /*    56     4 */

	/* size: 64, cachelines: 1, members: 8 */
	/* padding: 4 */
};

Still padded to a cacheline, but cleaner pattern. Thanks for the suggestion.

@nobu
Copy link
Member

nobu commented Jul 5, 2019

Nested struct for flags is not a common convention, but just some code use it to isolate from bit struct.
So you may place the flags at the same level as the other members, if you prefer.

@methodmissing
Copy link
Contributor Author

Thanks for the context and explanation again - indifferent for me - what do you prefer as committer?

@nobu
Copy link
Member

nobu commented Jul 6, 2019

I think this struct is small enough.

@methodmissing
Copy link
Contributor Author

OK to merge now without further changes?

@matzbot matzbot closed this in 612cad5 Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants