Skip to content
This repository has been archived by the owner on Oct 21, 2021. It is now read-only.

Remove vertex_index(::Integer), make special case for SimpleGraph #135

Merged
merged 2 commits into from Dec 21, 2014

Conversation

mlubin
Copy link
Contributor

@mlubin mlubin commented Dec 21, 2014

Thanks @sbromberger
Fixes #131
Fixes #127
Closes #133

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 6c0b52f on int_index into 6525b82 on master.

@mlubin
Copy link
Contributor Author

mlubin commented Dec 21, 2014

Implements the suggestion by @lindahua, so will merge soon unless there are any objections.

@@ -37,7 +35,10 @@ typealias ProvidedVertexType Union(Integer, KeyVertex, ExVertex)

function vertex_index{V}(v::V, g::AbstractGraph{V})
@graph_requires g vertex_list
return vertex_index(v, vertices(g))
if applicable(vertex_index, v)
return vertex_index(v)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since applicable makes for more complex generated code and there's only one case where it's true, perhaps just writing out two separate methods instead? (I think it's clearer and no increase in lines of code, too.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would make it impossible to extend KeyVertex. Is the code generation actually bad for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Double checked that it did not specialize how you'd hope (let me know if I did something wrong):

immutable A
    int::Int
end
immutable B
    int::Int
end

g(x) = x*x
f(::A, x::Int) = 2x
f(b::B) = b.int

a_only(a::A, x::Int) = f(a, g(x))
b_only(b::B, x::Int) = f(b)
function a_or_b{T}(v::T, x::Int)
    if applicable(f, v)
        return f(v)
    end
    return f(v, g(x))  
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what constitutes bad -- it (and method_exists) just doesn't specialize like I expected the first time I used it:
Using that sample code above:

code_native(a_or_b2, (A, Int))
    .section    __TEXT,__text,regular,pure_instructions
Filename: none
Source line: 2
    push    RBP
    mov RBP, RSP
    push    R15
    push    R14
    push    R12
    push    RBX
    sub RSP, 32
    mov RBX, RSI
    mov R14, RDI
    mov QWORD PTR [RBP - 64], 4
Source line: 2
    movabs  R12, jl_pgcstack
    mov RAX, QWORD PTR [R12]
    mov QWORD PTR [RBP - 56], RAX
    lea RAX, QWORD PTR [RBP - 64]
    mov QWORD PTR [R12], RAX
    vxorps  XMM0, XMM0, XMM0
    vmovups XMMWORD PTR [RBP - 48], XMM0
    movabs  RAX, 140405752947744
Source line: 2
    mov RDI, QWORD PTR [RAX]
    mov RAX, QWORD PTR [RDI + 8]
    movabs  RCX, 140405770133600
    mov RCX, QWORD PTR [RCX]
Source line: 2
    lea R15, QWORD PTR [RBP - 48]
    movabs  RDX, 140405903671888
Source line: 2
    mov QWORD PTR [RBP - 48], RCX
    mov QWORD PTR [RBP - 40], RDX
    mov RSI, R15
    mov EDX, 2
    call    RAX
    movabs  RCX, 140405753013296
    cmp RAX, RCX
    je  L218
Source line: 3
    movabs  RAX, allocobj
    mov EDI, 16
    call    RAX
    movabs  RCX, 140405864207744
    mov QWORD PTR [RAX], RCX
    mov QWORD PTR [RAX + 8], R14
    mov QWORD PTR [RBP - 48], RAX
    movabs  RAX, jl_apply_generic
    movabs  RDI, 140405793447904
    mov RSI, R15
    mov EDX, 1
    call    RAX
    jmpq    L240
Source line: 5
L218:   imul    RBX, RBX
    add RBX, RBX
    movabs  RAX, jl_box_int64
    mov RDI, RBX
    call    RAX
L240:   mov RCX, QWORD PTR [RBP - 56]
    mov QWORD PTR [R12], RCX
    add RSP, 32
    pop RBX
    pop R12
    pop R14
    pop R15
    pop RBP
    ret
julia> code_native(a_only, (A, Int))
    .section    __TEXT,__text,regular,pure_instructions
Filename: none
Source line: 1
    push    RBP
    mov RBP, RSP
Source line: 1
    imul    RSI, RSI
    lea RAX, QWORD PTR [RSI + RSI]
    pop RBP
    ret

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a specialization for KeyVertex and leave this as it is for extensibility. The slow case where we have to do a linear search on indices is already slow and shouldn't be affected by this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, makes sense to me.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b18c4bb on int_index into 6525b82 on master.

mlubin added a commit that referenced this pull request Dec 21, 2014
Remove vertex_index(::Integer), make special case for SimpleGraph
@mlubin mlubin merged commit e380d38 into master Dec 21, 2014
@mlubin mlubin deleted the int_index branch December 21, 2014 23:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incorrect behavior of vertex_index (in all versions) Int vertex indexing behavior is surprising
4 participants