Yicong-Huang commented on PR #57911:
URL: https://github.com/apache/spark/pull/57911#issuecomment-5259393597
@gaogaotiantian, you’re right to be skeptical!
> I think the benchmark diff is purely caused by re-ordering the if
statements. You are running benchmarks on cases where the fast path is taken
(fast because you just moved it to the top). What about the old benchmark?
Could you check the "slow" path when the input is not `str`? For example, what
if they are all `None`?
The improvement comes from short-circuiting the `element_conv` call on the
fast path. Also, isinstance may traverse the MRO, making it more expensive than
an exact `type()` check for built-in types such as str and bytes.
For the slow path, the additional cost is a 7–8 ns type check. I updated the
benchmarks to cover a range of distributions, including cases where 30%, 70%,
and 100% of values are already the expected str or bytes type. Most cases still
show a net improvement. In the extreme case where every value has the wrong
type and requires element_conv, the change adds approximately 4–9% overhead.
I also added `is None` as an earlier short-circuit because it is very
inexpensive. For string_scalar with 100% None values, the measured overhead is
0%.
> `str(s) is s` and `bytes(b) is b` (when `s` and `b` are `str` and `bytes`
respectively) - there's no copy involved.
My statement was wrong and you are correct that there is no copy. The
savings come from avoiding the method calls themselves and also `bytes()`,
which is relatively expensive.
> I believe this PR simply makes some cases faster and some slower by moving
around stuff. We can't add a bunch of benchmarks that are supposed to be better
by reordering to support the change.
That is true, but I would argue that `str` values for `StringType` and
`bytes` values for `BinaryType` are the common hot paths and are worth
prioritizing. For slow-path values that require conversion to the target type,
I would also like to discuss whether we should continue supporting these
implicit coercions, although that is a separate topic.
> Also for every `type()` check introduced, we add an extra function call
overhead.
Yes, but my benchmarks show that `type()` is inexpensive compared with
`element_conv`, `str()`, and especially `bytes()`. I believe the fast-path
benefit justifies the additional check.
> I'm not saying we should not prioritize `str -> str` path. It might be
real that this is a more common case. But this PR is a heuristic, not an
optimization. We should at least be clear about that and leave it in the
description and comments - that we believe in most UDF cases, users return
`str` when they use `StringType`, so we do a fast path check first. Not sure if
we have enough support data for that.
However, I'm a bit skeptical for this kind of heuristic in general. This
works slightly better for some cases and slightly worse in others. We could
potentially be swamped in a discussion about "how our users use our library".
Agreed. I updated the description and comments to make this trade-off
explicit.
One additional point: the updated benchmarks show improvements of up to 40%
for byte arrays, 30% for scalar bytes, and approximately 20% for string arrays.
I think the improvements are worth the trade off. I also see end-to-end
improvements in real jobs that are output heavy, which initially points me to
optimize this code path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
