search

aspnet / JitBench

Repo for testing JIT stuff
Other
15 stars 24 forks source link

Fix MusicStore JitBench in Docker #99

Closed luhenry closed 5 years ago

luhenry commented 5 years ago

We need to be extra specific on which address Kestrel should listen, or it won't be able to bind and connect.

luhenry commented 5 years ago

/cc @kouvel @sergiy-k

sergiy-k commented 5 years ago

Thank you for fixing it!

kouvel commented 5 years ago

LGTM. Note that JitBench in the CoreCLR repo pulls in a specific commit hash from the tiered_compilation_demo branch I believe, which has changes to output that are expected. May need to make the change to that branch as well, and update the commit hash used by the CoreCLR repo.

luhenry commented 5 years ago

@kouvel I will make sure to bump the dependency in https://github.com/dotnet/coreclr/pull/24248

kouvel commented 5 years ago

Ok just to clarify the output generated from the tiered_compilation_demo branch is different from the output generated by the rel/2.0.0 branch, and the output from the tiered_compilation_demo branch is expected by the CoreCLR's JitBench project. So you may need to merge this change into the tiered_compilation_demo branch as well and use that commit's ref in CoreCLR.

luhenry commented 5 years ago

@kouvel I am surprised CoreCLR depends on tiered_compilation_demo. When I compared which commits where checked out locally, it was pointing to rel/2.0.0 and not tiered_compilation_demo. Moreover, it seems tiered_compilation_demo removes aspnet-generatestore.sh which coreclr depends on (https://github.com/aspnet/JitBench/compare/6bee730486f272d31f23f1033225090511f856f3...tiered_compilation_demo).

kouvel commented 5 years ago

Ah I got confused between different changes, my bad. You're right, the commit currently used in CoreCLR points at rel/2.0.0 latest.

luhenry commented 5 years ago

@rynowak is that good to be merged, or do you still need me to look into https://github.com/aspnet/JitBench/pull/99#pullrequestreview-230820330? I don't have the credentials to merge this PR, so if it's good to go, could you please merge it? Thank you! :)

rynowak commented 5 years ago

I'm happy to merge this since I'm just maintaining this for CLR folks to use. I don't have a full list of who's using what from this repo, so there's the possibility for this to need more discussion afterr the fact.