add faster done for strings

[KristofferC]

Fixes #18081

Uses @TotalVerb's suggestion in #18081 (comment)

[nalimilan]

Candidate for backport too?

[KristofferC]

there are not many string benchmarks but anyway... @nanosoldier runbenchmarks("string" && "problem", vs = ":master")

[nanosoldier]

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels

[KristofferC]

Oh.. Used the wrong invocation..

[KristofferC]

@nanosoldier runbenchmarks("string" || "problem", vs = ":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

[KristofferC]

I cannot reproduce the join regression and it has been noisy on many other benchmark runs. I can reproduce the performance gain in for example lstrip as in #18081.

[tkelman]

Does this do the right thing for unicode? Could really use more tests on the correctness here in corner cases.

[KristofferC]

https://github.com/JuliaLang/julia/blob/master/test/strings/basic.jl#L255?

[tkelman]

Won't endof(s) and endof(s.data) behave differently if the string ends in continuation characters?

julia/base/strings/string.jl

Lines 36 to 43 in 28f1306

	function endof(s::String)
	d = s.data
	i = length(d)
	@inbounds while i > 0 && is_valid_continuation(d[i])
	i -= 1
	end
	i
	end

[KristofferC]

ok

julia> for x in String([0xcf, 0x83, 0x83, 0x83, 0x83])
             print(x)
             end
σ
------------------------------------------------------------------------------------------
UnicodeError                                            Stacktrace (most recent call last)
[#3] — anonymous
       ⌙ at <missing>:?

[#2] — next;
       ⌙ at string.jl:92 [inlined]

[#1] — slow_utf8_next(::Array{UInt8,1}, ::UInt8, ::Int64)
       ⌙ at string.jl:67

UnicodeError: invalid character index

[KristofferC]

Should have a fix.

[KristofferC]

I thought that an iterator that carried along the endof value would work but it seems that there are many places in base that assumes that start(::String) is an int. So let's revert + add the test that failed.

[nalimilan]

Looks like (part of) the issue is that in lstrip, endof is called on each iteration even if done is inlined. OTC, with a simple function, it's called only once (which means the compiler effectively does the hoisting you proposed to do manually):

function f(s::String)
    for c in s
        c == 'a' && return true
    end
    return false
end

We should investigate the reason for the differences. Maybe the compiler cannot guarantee that s isn't modified in complex functions? Could @pure help?

[TotalVerb]

Do we really need this to work on invalid UTF8? The added test has an overlong encoding, which is not permitted.

[TotalVerb]

Passing in additional information with next would probably be detrimental to performance. It's almost certainly better to go with this approach. The only requirement is that next return an index past the end of the underlying data, which the only sane implementations I can think of (including the current implementation) indeed do. I think we should make this guarantee (perhaps undocumented) and test it.

[TotalVerb]

@nalimilan It is not possible to force hoisting here because endof is not actually inlined, so LLVM can't do anything. The function is actually quite complex, involving vector dereference in a loop. We could try to force @inline it, but I don't know if that is profitable. In the interim, we should just merge #18280.

[nalimilan]

I'm not opposed to merging #18280, but I still think we should investigate that hoisting issue fpr other string types. I didn't say that endof was inlined, but that done was (or sometimes should), which allows the compiler to notice that endof(s) doesn't need to be called in each loop since it's constant (s is immutable).

[TotalVerb]

The problem is not related to done, which is inlined without issue; if endof is not inlined, then LLVM only sees call i64 @jlsys_endof_63780(%jl_value_t* %0), which it cannot hoist out of the loop because the effects are unpredictable. The only hope for loop hoisting would be for endof to be inlined first.

As far as I can tell, there is no loop hoisting in the example you gave,

function f(s::String)
    for c in s
        c == 'a' && return true
    end
    return false
end

as the LLVM code's loop condition contains a call to endof:

L2:                                               ; preds = %L22, %top
  %"#temp#.0" = phi i64 [ 1, %top ], [ %"#temp#1.sroa.4.0", %L22 ]
  %12 = call i64 @jlsys_endof_63780(%jl_value_t* %0)
  %13 = icmp slt i64 %12, %"#temp#.0"
  br i1 %13, label %L33, label %if

[nalimilan]

Woops, looks like I misread the generated code. I don't remember how, though.

The only hope for loop hoisting would be for endof to be inlined first.

Yet shouldn't @pure have this effect? Or maybe it doesn't work because the contents of String are actually mutable?

[TotalVerb]

As far as I know, LLVM does not get any information assuring that the function call is pure. I don't know if it supports callsite purity annotations, but even if does, it may be non-trivial to add them.

[vtjnash]

Yes, @pure could have this effect. LLVM has many callsite purity annotations and it's easy to add them (the most non-trivial part is probably picking the right ones :P). PRs are welcome.

[nalimilan]

Easier said than done (for me at least :-).