Skip to content

Conversation

@vtjnash
Copy link
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
Member Author

vtjnash commented Nov 15, 2019

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


static void throw_ovf(int should_malloc, void *desc, jl_datatype_t* st, int offset)
{
if (should_malloc)
Copy link
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
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
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
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

@vtjnash vtjnash merged commit 1e39c69 into master Nov 18, 2019
@vtjnash vtjnash deleted the jn/layout-reorg branch November 18, 2019 20:30
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.

5 participants