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

pack more fields in datatype #41018

Merged
merged 2 commits into from
Jun 7, 2021
Merged

pack more fields in datatype #41018

merged 2 commits into from
Jun 7, 2021

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented May 31, 2021

As Jeff noted recently, more fields can be moved to reduce the data size (I think removing .names had the biggest effect on sysimg size, but all seemed helpful). I've also packed some Bool fields too.

@vtjnash vtjnash requested a review from JeffBezanson May 31, 2021 02:07
@KristofferC
Copy link
Sponsor Member

@nanosoldier runtests(ALL, vs = ":master")

Just curious about the "damage".

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented May 31, 2021

Looks like a couple "key" packages (SymbolServer, AxisArrays, JuliaInterpreter), but not a major impact.

function datatype_min_ninitialized(t::DataType)
isabstracttype(t) && return 0
if t.name === NamedTuple_typename
names, types = t.parameters[1], t.parameters[2]
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We could just call fieldcount in this case.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Hm, I guess not, since we want 0 instead of an error for abstract types...

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.

yes, that is why it is reimplemented here

STATIC_INLINE jl_sym_t *jl_field_name(jl_datatype_t *st, size_t i) JL_NOTSAFEPOINT
{
return (jl_sym_t*)jl_svecref(jl_field_names(st), i);
return st->name->names;
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Maybe assert here that it's not a named tuple?

Copy link
Sponsor Member Author

@vtjnash vtjnash Jun 1, 2021

Choose a reason for hiding this comment

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

I figured it would assert due to the null pointer anyways, so it was unnecessary to enforce it (edit: and it is also applicable to Tuple)

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.

4 participants