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

Remove built in methods' function wrapper. #719

Closed
wants to merge 2 commits into from

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Aug 19, 2018

I removed the function wrapper for most class' built-in methods because they are marked as one of the hotspots when profiling. And for most cases, the wrapper function is unnecessary anyway.

@st0012 st0012 added this to the version 0.1.11 milestone Aug 19, 2018
@st0012 st0012 self-assigned this Aug 19, 2018
@ghost ghost added the in progress label Aug 19, 2018
@codecov
Copy link

codecov bot commented Aug 19, 2018

Codecov Report

Merging #719 into master will decrease coverage by 0.28%.
The diff coverage is 87.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #719      +/-   ##
==========================================
- Coverage   79.72%   79.44%   -0.29%     
==========================================
  Files          53       53              
  Lines        7355     7252     -103     
==========================================
- Hits         5864     5761     -103     
- Misses       1240     1244       +4     
+ Partials      251      247       -4
Impacted Files Coverage Δ
vm/string.go 95.51% <ø> (-0.05%) ⬇️
vm/channel.go 88.46% <100%> (-1.2%) ⬇️
vm/boolean.go 91.3% <100%> (-1.01%) ⬇️
vm/concurrent_rw_lock.go 90.9% <100%> (-0.76%) ⬇️
vm/go_object.go 12.69% <15.38%> (-6.42%) ⬇️
vm/concurrent_array.go 76.92% <62.5%> (-2.03%) ⬇️
vm/integer.go 73.33% <66.66%> (-0.53%) ⬇️
vm/go_map.go 67.27% <71.42%> (-3.22%) ⬇️
vm/file.go 72.51% <74.33%> (-1.21%) ⬇️
vm/float.go 80.43% <78.88%> (-0.82%) ⬇️
... and 9 more

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 f54dc3e...4a800d2. Read the comment docs.

Copy link
Member

@hachi8833 hachi8833 left a comment

Choose a reason for hiding this comment

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

I think the changes are nice!

@st0012
Copy link
Member Author

st0012 commented Oct 18, 2018

@hachi8833 do you have time to help me rebase this? I'm afraid I might miss some of your changes if I do it myself 😓

@hachi8833
Copy link
Member

OK, I'll look into it for rebasing.

@hachi8833
Copy link
Member

Could you check #747 for the workaround of rebasing? @st0012

@hachi8833
Copy link
Member

I think you can close the PR because #747 has been merged. Thanks! @st0012

@st0012 st0012 closed this Nov 25, 2018
@ghost ghost removed the in progress label Nov 25, 2018
@st0012 st0012 deleted the Remove-builtin-method-function branch November 25, 2018 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants