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

only store JIT function names, not full prototypes #22678

Merged
merged 2 commits into from
Jul 14, 2017
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jul 4, 2017

this saves a bit of memory and shaves a bit off of startup time

@vtjnash vtjnash requested a review from maleadt July 4, 2017 17:13
@vtjnash vtjnash force-pushed the jn/cgctx-globals branch from e5232c5 to de817cc Compare July 5, 2017 04:28
@@ -1313,9 +1314,8 @@ static jl_method_instance_t *jl_get_unspecialized(jl_method_instance_t *method)

// this compiles li and emits fptr
extern "C"
jl_generic_fptr_t jl_generate_fptr(jl_method_instance_t *li, void *_F, size_t world)
jl_generic_fptr_t jl_generate_fptr(jl_method_instance_t *li, const char *F, size_t world)
Copy link
Member

Choose a reason for hiding this comment

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

fname for consistency? Happens in lots of places.

Copy link
Member

Choose a reason for hiding this comment

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

We're kinda fuzzy on F/function/llvmf/specf/f naming too...

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, we just kind of pick whichever one is appropriate given whichever other variables needed to also appear. I'm not sure it's worth too much effort to try to do a sweep through and make them that much more consistent. I'm planning on eliminating these names anyways and switching just to using the fptr field directly. This PR is just part of a transitional state to slowly decouple and remove that global state.

@vtjnash vtjnash force-pushed the jn/cgctx-globals branch from de817cc to 3ec5dc0 Compare July 5, 2017 18:17
@vtjnash vtjnash merged commit 615a7ba into master Jul 14, 2017
@vtjnash vtjnash deleted the jn/cgctx-globals branch July 14, 2017 23:17
@tkelman
Copy link
Contributor

tkelman commented Jul 15, 2017

really should have run nanosoldier here before merging

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.

3 participants