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

Compiler: Add "Top 10 slowest modules" to --stats output #12942

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Compiler: Add "Top 10 slowest modules" to --stats output #12942

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bfafd46
Compiler: Add "Top 10 slowest modules" to --stats output
z64 Jan 11, 2023
596fbb7
Remove unused string justification that is probably not needed
z64 Jan 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Compiler: Add "Top 10 slowest modules" to --stats output
  • Loading branch information
@z64
z64 committed Jan 11, 2023
commit bfafd46d1bcc4e8985131d346edea8a7e6c60fdd
35 changes: 30 additions & 5 deletions src/compiler/crystal/compiler.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,19 +470,31 @@ module Crystal
pr, pw = IO.pipe
spawn do
pr.each_line do |line|
unit = JSON.parse(line)
reused << unit["name"].as_s if unit["reused"].as_bool
unit_info = JSON.parse(line)
reused << unit_info["name"].as_s if unit_info["reused"].as_bool

module_idx = unit_info["idx"].as_i
slice.update(module_idx) do |unit|
unit.compilation_time = unit_info["time"].as_f.seconds
unit
end
Comment on lines +477 to +480
Copy link
Contributor

@HertzDevil HertzDevil Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

unit is a reference, so I believe direct mutation works and there is no need to do it like a value (unless there is some complication with forking)

Copy link
Contributor Author

@z64 z64 Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Pages in forked processes are copy-on-write by default. And the pages allocated by bdwgc are not marked as MEM_SHARED (probably for good reason, I'd wager). That is why this pipe mechanism is used, as a safe vehicle to share memory.

Edit: Sorry, mistook the question; Yes - in this context we are in the parent thread, so mutating should be fine here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you still going to address this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HertzDevil No, make any changes you see fit.


@progress_tracker.stage_progress += 1
end
end
end

codegen_process = Process.fork do
pipe_w = pw
slice.each do |unit|
slice.each_with_index do |unit, idx|
unit.compile
if pipe_w
unit_json = {name: unit.name, reused: unit.reused_previous_compilation?}.to_json
unit_json = {
name: unit.name,
reused: unit.reused_previous_compilation?,
idx: idx,
time: unit.compilation_time.total_seconds,
}.to_json
pipe_w.puts unit_json
end
end
Expand Down Expand Up @@ -547,6 +559,16 @@ module Crystal
puts " - #{unit.original_name} (#{unit.name}.bc)"
end
end

if units.size != reused.size
puts
puts("Top 10 slowest modules:")
units.sort_by! { |u| u.compilation_time * -1 }
units.first(10).each do |unit|
time_str = unit.compilation_time.to_s.ljust(18, ' ')
z64 marked this conversation as resolved.
Show resolved Hide resolved
puts " - #{unit.compilation_time} #{unit.original_name} (#{unit.name}.bc)"
z64 marked this conversation as resolved.
Show resolved Hide resolved
end
end
end

getter(target_machine : LLVM::TargetMachine) do
Expand Down Expand Up @@ -631,6 +653,7 @@ module Crystal
getter original_name
getter llvm_mod
getter? reused_previous_compilation = false
property compilation_time : Time::Span = Time::Span::ZERO
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be a property! instead

@object_extension : String

def initialize(@compiler : Compiler, program : Program, @name : String,
Expand Down Expand Up @@ -661,7 +684,9 @@ module Crystal
end

def compile
compile_to_object
@compilation_time = Time.measure do
compile_to_object
end
end

private def compile_to_object
Expand Down