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

datatype: reorganize layout calculation code #33724

Merged
merged 3 commits into from
Nov 18, 2019
Merged

datatype: reorganize layout calculation code #33724

merged 3 commits into from
Nov 18, 2019

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Oct 30, 2019

The code felt a bit disorganized to me, especially after #32448, and with the upcoming desire to eventually merge #18632. I think this will make it clearer when adopting further changes by changing where some of the early-return occurs and thus also removing some indentation (hence also, diff readability may be improved by ignoring whitespace).

@vtjnash vtjnash force-pushed the jn/layout-reorg branch 2 times, most recently from acda06b to 85c5468 Compare November 1, 2019 03:27
vtjnash and others added 3 commits November 4, 2019 10:56
This lets us scan a datatype slightly easier,
and opens up a future possibility where we don't have a one-to-one
relationship between fields and contained pointers.
@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 15, 2019

Seems unlikely to be noticeable, but just to check: @nanosoldier runbenchmarks(ALL, vs=":master")

@@ -296,165 +296,183 @@ static int references_name(jl_value_t *p, jl_typename_t *name) JL_NOTSAFEPOINT
return 0;
}

static void throw_ovf(int should_malloc, void *desc, jl_datatype_t* st, int offset)
{
if (should_malloc)
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should_free?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it could. It's currently just carrying the same name along.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you care?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so important I guess.

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants