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

Reduce build time with minor reference refactor #2679

Merged
merged 1 commit into from
Aug 29, 2017

Conversation

xinranw
Copy link

@xinranw xinranw commented Aug 4, 2017

Adding a self. reference to a instance variable reduced the compile
time significantly (over 10x improvement). A single line evaluation of a
variable was further broken out into separate steps to reduce the
compile time as well.

This cuts at least 2 seconds from the build time

Compile times were analyzed with the following bash command:
xcodebuild -scheme Charts -sdk iphonesimulator PLATFORM_NAME=iphonesimulator clean build OTHER_SWIFT_FLAGS="-Xfrontend -debug-time-function-bodies" 2>&1 | grep -e [0-9][0-9].[0-9].ms | sort -nr

Compile times before the change:
before-change

Compile times after the change:
after-change

@xinranw xinranw changed the title Reduce build time with minor reference change Reduce build time with minor reference refactor Aug 4, 2017
@codecov-io
Copy link

codecov-io commented Aug 4, 2017

Codecov Report

Merging #2679 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2679      +/-   ##
==========================================
- Coverage   19.65%   19.65%   -0.01%     
==========================================
  Files         112      112              
  Lines       13717    13715       -2     
==========================================
- Hits         2696     2695       -1     
+ Misses      11021    11020       -1
Impacted Files Coverage Δ
.../Charts/Renderers/HorizontalBarChartRenderer.swift 0% <0%> (ø) ⬆️
...ource/Charts/Renderers/ChartDataRendererBase.swift 47.05% <0%> (-2.95%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e72f280...80a45c0. Read the comment docs.

@liuxuan30
Copy link
Member

Hi there, thanks for the input. Do you have any doc or post could explain this change?

@xinranw
Copy link
Author

xinranw commented Aug 9, 2017

The part at the bottom of this article references a similar problem, where chaining operations can sometimes cause dramatic increases in compile time (also referenced by an Apple build engineer at WWDC).

For the self. addition that actually accounted for the majority of the build time improvement...I honestly don't have a good answer. Based on articles such as this and this, it seems a lot of the optimizations that others have found can be somewhat arbitrary. I ended up just trying various suggestions and noticed that adding the self. made a 1000x improvement. I wonder if this change is somehow helping the compiler figure out the reference to viewPortHandler (perhaps since it's a inherited property from the Renderer class.

@xinranw
Copy link
Author

xinranw commented Aug 14, 2017

Bump

@liuxuan30
Copy link
Member

liuxuan30 commented Aug 21, 2017

Thank you for your information. I'm fine with this PR, but also needs other peoples' review.

BTW, if it helps, do we need to change the entire code base?

@xinranw
Copy link
Author

xinranw commented Aug 21, 2017

There are certainly other parts of the code that can be optimized but these two lines were taking the longest amount of time (over 2 seconds if being used as a cocoapod). So I only tackled these two as the easiest way to cut down a somewhat significant part of the build time.

return data.entryCount < Int(CGFloat(dataProvider?.maxVisibleCount ?? 0) * (viewPortHandler?.scaleX ?? 1.0))

let maxVisibleCount = CGFloat(dataProvider?.maxVisibleCount ?? 0)
let scale = self.viewPortHandler?.scaleX ?? 1.0
Copy link
Member

Choose a reason for hiding this comment

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

I got self is improving the speed. But can we just uses self, without using two instants? like

return data.entryCount < Int(CGFloat(dataProvider?.maxVisibleCount ?? 0) * (self.viewPortHandler?.scaleX ?? 1.0))

I assume it has the same effect?

Copy link
Author

Choose a reason for hiding this comment

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

The self change does account for most of the improvements. Chained operations do tack on some build time increase but it's probably not significant for this case. Happy to go ahead and update with that change.

Adding a `self.` reference to a instance variable reduced the compile
time significantly (over 10x improvement). A single line evaluation of a
variable was further broken out into separate steps to reduce the
compile time as well.
@liuxuan30 liuxuan30 merged commit 69cdd84 into ChartsOrg:master Aug 29, 2017
@xinranw xinranw deleted the reduce-build-time branch August 30, 2017 00:15
@danielgindi
Copy link
Collaborator

Cool! Thanks!

PeterSrost pushed a commit to sokol8/Charts that referenced this pull request Oct 31, 2018
Reduce build time with minor reference refactor
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.

None yet

4 participants