-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
pack more fields in datatype #41018
Conversation
@nanosoldier Just curious about the "damage". |
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt |
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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
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.