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

getfield for Node ptr #144

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

Conversation

chris-b1
Copy link

This is the same issue/approach as #142

I'm doing some profiling on XLSX.jl and this picks up a nice little win

# master
julia> @btime XLSX.readtable(f, "Sheet1")
  341.240 ms (3018190 allocations: 98.87 MiB

# PR
julia> @btime XLSX.readtable(f, "Sheet1")
  284.024 ms (1940230 allocations: 54.22 MiB)

@codecov
Copy link

codecov bot commented May 16, 2020

Codecov Report

Merging #144 into master will decrease coverage by 0.24%.
The diff coverage is 88.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
- Coverage   93.55%   93.30%   -0.25%     
==========================================
  Files           7        7              
  Lines         760      762       +2     
==========================================
  Hits          711      711              
- Misses         49       51       +2     
Impacted Files Coverage Δ
src/node.jl 93.46% <88.57%> (-0.48%) ⬇️

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 501c2a6...84c32ef. Read the comment docs.

@chris-b1
Copy link
Author

chris-b1 commented Jun 2, 2020

gentle ping @bicycle1885 - any changes you'd like to see here?

@aminya
Copy link

aminya commented Jun 15, 2020

@chris-b1 He does maintain the package on GitHub. You should email him in person and let him know that there is a pull request. I wish we had moved this repository to an organization to let others continue developing the package.

@bicycle1885
Copy link
Member

@chris-b1 I appreciate your suggestion. However, I discovered this will be fixed by the improved compiler and what we need to do is just waiting for the next release of Julia (i.e., Julia 1.6). For example, lots of instructions related to getproperty in Julia 1.5.3 are optimized out in the latest nightly build of Julia as follows:

julia> code_native(hasparentnode, (EzXML.Node,))
        .text
; ┌ @ node.jl:616 within `hasparentnode'
; │┌ @ node.jl:398 within `getproperty'
        movq    (%rdi), %rax
; │└
; │┌ @ operators.jl:204 within `!='
; ││┌ @ pointer.jl:155 within `==' @ promotion.jl:410
        cmpq    $0, 40(%rax)
        setne   %al
; │└└
        retq
        nopl    (%rax)
; └

julia> versioninfo()
Julia Version 1.7.0-DEV.6
Commit 5f1b21b357* (2020-12-09 16:06 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: AMD Ryzen 9 3950X 16-Core Processor
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.0 (ORCJIT, znver2)
Environment:
  JULIA_PROJECT = @.
julia> code_native(hasparentnode, (EzXML.Node,))
        .text
; ┌ @ node.jl:615 within `hasparentnode'
        pushq   %r15
        pushq   %r14
        pushq   %rbx
        subq    $48, %rsp
        vxorps  %xmm0, %xmm0, %xmm0
        movq    $0, 32(%rsp)
        movq    %fs:0, %rbx
        leaq    16(%rsp), %rdi
        movq    %rsp, %r14
        movabsq $140301301986384, %rcx  # imm = 0x7F9A71443450
; │ @ node.jl:616 within `hasparentnode'

... and many instructions!

julia> versioninfo()
Julia Version 1.5.3
Commit 788b2c77c1 (2020-11-09 13:37 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: AMD Ryzen 9 3950X 16-Core Processor
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-9.0.1 (ORCJIT, znver2)
Environment:
  JULIA_EDITOR = "/usr/share/code/code"
  JULIA_NUM_THREADS = 

Given that the current performance is not devastating and the suggested change affects many lines, I think it would be better to stay and wait for Julia 1.6.

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.

3 participants