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.

Sign up for GitHub

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

Jump to bottom

Remove JIT icall hashing by name and AOT JIT icall hashing. #14644

Merged
merged 1 commit into from May 30, 2019
Merged

Remove JIT icall hashing by name and AOT JIT icall hashing. #14644

merged 1 commit into from May 30, 2019

Conversation

jaykrell
Copy link
Contributor

@jaykrell jaykrell commented May 25, 2019 •
edited

This time I did not rename MONO_PATCH_INFO_JIT_ICALL_ADDR and MONO_PATCH_INFO_JIT_ICALL_ADDR_NOCALL.

Could go either way.
Could change them in a separate PR.

@jaykrell jaykrell force-pushed the nojiticallname branch 2 times, most recently from 2136e9d to 7b5b82d Compare May 25, 2019 04:14
@jaykrell jaykrell changed the title JIT icall hash reduction. Remove JIT icall hashing by name. May 25, 2019
@jaykrell jaykrell force-pushed the nojiticallname branch 2 times, most recently from bb29d30 to 967937b Compare May 25, 2019 08:15
@jaykrell
Copy link
Contributor Author

@monojenkins build failed

1 similar comment
@jaykrell
Copy link
Contributor Author

@monojenkins build failed

@jaykrell
Copy link
Contributor Author

Windows is failing for lack of cmake buiding BTLS.

@jaykrell
Copy link
Contributor Author

@monojenkins build failed

@jaykrell jaykrell marked this pull request as ready for review May 25, 2019 18:02
@jaykrell
Copy link
Contributor Author

@monojenkins build failed

jaykrell
jaykrell commented May 25, 2019
View reviewed changes
mono/metadata/icall.c Outdated Show resolved Hide resolved
mono/mini/aot-runtime.c Outdated Show resolved Hide resolved
@jaykrell
Copy link
Contributor Author

@monojenkins build failed

1 similar comment
@jaykrell
Copy link
Contributor Author

@monojenkins build failed

@jaykrell
Copy link
Contributor Author

/cc @vargaz

jaykrell
jaykrell commented May 26, 2019
View reviewed changes
mono/metadata/icall.c Outdated Show resolved Hide resolved
jaykrell
jaykrell commented May 26, 2019
View reviewed changes
mono/mini/aot-runtime.c Outdated Show resolved Hide resolved
@jaykrell jaykrell force-pushed the nojiticallname branch 2 times, most recently from 7763986 to 9e8689b Compare May 26, 2019 23:20
@jaykrell
Copy link
Contributor Author

@monojenkins build failed

1 similar comment
@jaykrell
Copy link
Contributor Author

@monojenkins build failed

@jaykrell jaykrell force-pushed the nojiticallname branch 3 times, most recently from 0609d99 to 4aae294 Compare May 29, 2019 01:13
@jaykrell
Copy link
Contributor Author

@monojenkins build failed

vargaz
vargaz reviewed May 29, 2019
View reviewed changes
mono/mini/aot-runtime.c
g_error ("Unknown relocation '%p'\n", ji->data.target);
target = NULL;
target = mono_arch_load_function (ji->data.jit_icall_id);
g_assertf (target, "Unknown relocation '%p'\n", ji->data.target);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs some kind of ifdef or needs to be added for all architectures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm "rewriting" it..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It bugged me to lose the inlining. I think the function was provided for all architectures that support AOT, as well as PowerPC, but anyway, the rewrite is tweaked to handle architectures with no extra functions.

Perhaps I should have removed only one hash table in this PR and left the AOT one asis. I could do that, though still heading toward the same place, if ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another solution is put back the AOT registration, into the struct/array, and just index into that (mono_find_jit_icall).

@jaykrell
Copy link
Contributor Author

@monojenkins build failed

1 similar comment
@jaykrell
Copy link
Contributor Author

@monojenkins build failed

@jaykrell jaykrell changed the title Remove JIT icall hashing by name. Remove JIT icall hashing by name and AOT JIT icall hashing. May 29, 2019
@jaykrell
Copy link
Contributor Author

@monojenkins build failed

vargaz
vargaz reviewed May 29, 2019
View reviewed changes
mono/mini/mini-x86.h Outdated
@@ -350,5 +350,10 @@ mono_x86_start_gsharedvt_call (GSharedVtCallInfo *info, gpointer *caller, gpoint
CallInfo*
mono_arch_get_call_info (MonoMemPool *mp, MonoMethodSignature *sig);

#define MONO_ARCH_AOT_ICALLS \
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous approach was fine, why do these need to be inlined ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just keeping it optimal where easy.
You prefer the per-arch helper function, nested switch instead of one switch?
I can put that back and fill in the other architectures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Registration is also not terrible, then in the switch just index into the array.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the helper function or the registration, this is only run a few times on startup, there is no need to make it optimal.

Copy link
Contributor Author

@jaykrell jaykrell May 29, 2019 •
edited

Choose a reason for hiding this comment

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

I'm fine either way. The helper function could also be inline in the .h.
Anything is better than a hashtable imho.
Optimal is also smaller and pays recurring dividends, albeit small ones. (smaller runtime, faster to install, and start, even if the code runs rarely/never)

I think when I first did this I didn't realize the connections, so I had redundantly optimized registration, and then didn't use the registration.

The registration also doesn't need the strings.

Note that I removed the ifdef gsharedvt from the helpers, that was in registration.
It isn't really conditional, as the current system is configured.

@jaykrell jaykrell force-pushed the nojiticallname branch 2 times, most recently from c12cc75 to d4920fc Compare May 29, 2019 19:43
@jaykrell
Copy link
Contributor Author

Might need the stub in mini-wasm.c. We'll see if CI says.

@jaykrell
Copy link
Contributor Author

@monojenkins build failed

@jaykrell
Copy link
Contributor Author

Yep. /mnt/jenkins/workspace/test-mono-pull-request-wasm/mono/mini/aot-runtime.c:5394: undefined reference to mono_arch_load_function'`

@jaykrell
Copy link
Contributor Author

commits after approval:

  • add wasm and mipsv stubs
  • squash it all into one

@jaykrell
Copy link
Contributor Author

@monojenkins build failed

@jaykrell
Copy link
Contributor Author

(accidentally put change in wrong PR, undone)

@jaykrell
Copy link
Contributor Author

@monojenkins build failed

2 similar comments
@jaykrell
Copy link
Contributor Author

@monojenkins build failed

@jaykrell
Copy link
Contributor Author

@monojenkins build failed

@jaykrell
Copy link
Contributor Author

jaykrell commented May 30, 2019 •
edited

Windows x64 FullAOT crashed and there are no symbols in the stack.
Let's see if retry works.

@jaykrell
Copy link
Contributor Author

@monojenkins build failed

@jaykrell
Copy link
Contributor Author

Could use symbolic Windows stacks for the flakes. :(

@akoeplinger akoeplinger merged commit e2d6361 into mono:master May 30, 2019
EgorBo pushed a commit to EgorBo/mono that referenced this pull request Jun 3, 2019
@jaykrell @EgorBo
Remove JIT icall hashing by name and AOT JIT icall hashing. (mono#14644)
9d5bf6e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vargaz vargaz vargaz approved these changes

@BrzVlad BrzVlad Awaiting requested review from BrzVlad

@lambdageek lambdageek Awaiting requested review from lambdageek

@lewurm lewurm Awaiting requested review from lewurm

@thaystg thaystg Awaiting requested review from thaystg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jaykrell @vargaz @akoeplinger