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

Split the big jit files to allow better GitHub experience. #13359

Open
sandreenko opened this issue Sep 4, 2019 · 5 comments
Open

Split the big jit files to allow better GitHub experience. #13359

sandreenko opened this issue Sep 4, 2019 · 5 comments
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@sandreenko
Copy link
Contributor

GitHub doesn't support showing huge files, for example, https://github.com/dotnet/coreclr/blob/master/src/jit/flowgraph.cpp has 25680 lines, but GitHub will show you only the first 14908. And it doesn't show any warning that the shown part is not full.

The same happens with reviewing diffs.

GitHub limits are not clear, https://help.github.com/en/articles/limits-for-viewing-content-and-diffs-in-a-repository tells about 1MB for text files, but flowgraph is smaller, I wrote them a question about code file sizes limits.

Once we get the actual limits we will be able to determinate the list of files that we need to split.

See also disscusion in dotnet/coreclr#26392 (comment).

cc @dotnet/jit-contrib

category:implementation
theme:jit-coding-style
skill-level:beginner
cost:medium

@danmoseley
Copy link
Member

Browser issue? I see all 25K lines at https://github.com/dotnet/coreclr/blob/master/src/jit/flowgraph.cpp. It takes time though and that doesn't mean smaller isn't better.

@Joe4evr
Copy link
Contributor

Joe4evr commented Sep 15, 2019

There was a discussion somewhat similar to this back when this repo was still new: https://github.com/dotnet/coreclr/issues/408#issuecomment-77922359

@sandreenko
Copy link
Contributor Author

I got the answer from the GitHub support:

We currently limit blob displaying to around 500kb of data, anything more we truncate in the UI. You can view the raw by clicking through.

So I think it would be rational to make a 400kb limit for all files that we have, it means we need to split 9 files:

10/14/2019  05:29 PM           413,140 emitarm64.cpp
10/14/2019  05:29 PM           435,201 lsra.cpp
10/24/2019  03:32 PM           447,708 compiler.h <- header!
10/14/2019  05:29 PM           451,531 codegencommon.cpp
10/24/2019  03:32 PM           462,582 emitxarch.cpp
10/24/2019  03:32 PM           637,322 gentree.cpp
10/24/2019  03:32 PM           734,066 morph.cpp
10/24/2019  03:32 PM           823,537 importer.cpp
10/24/2019  03:32 PM           920,170 flowgraph.cpp

@mikedn
Copy link
Contributor

mikedn commented Oct 25, 2019

Any idea how they can be split in a rational manner (e.g. not by cutting it in half and having something like importer1.cpp and importer2.cpp 😁)?

For flowgraph.cpp I'm sure we can find code in it that's not actually flowgraph related and perhaps that there's enough of it to move it somewhere else and matter. Not sure about the rest.

@sandreenko
Copy link
Contributor Author

Any idea how they can be split in a rational manner (e.g. not by cutting it in half and having something like importer1.cpp and importer2.cpp 😁)?

For flowgraph.cpp I'm sure we can find code in it that's not actually flowgraph related and perhaps that there's enough of it to move it somewhere else and matter. Not sure about the rest.

I think it should be individual in each case, and I think compiler.h is the biggest problem.

For example, for importer, it is probably enough to extract impImportBlockCode to solve that (it is 5000+ lines function).

Also, we can extract Compiler::verification logic or intrinsic functions.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@BruceForstall BruceForstall added the JitUntriaged CLR JIT issues needing additional triage label Oct 28, 2020
@sandreenko sandreenko removed the JitUntriaged CLR JIT issues needing additional triage label Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

No branches or pull requests

6 participants