Hashing closures is slow

[dnfield]

The framework has a relatively hot path on build when using text widgets that calls this:

https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/painting/binding.dart#L164

With a tearoff closure from a mixin. This ends up computing the hash code of the closure, which goes through this path in the VM, which is bubbling up in profiling as taking significant time:

sdk/runtime/vm/object.cc

Lines 9525 to 9532 in 7359264

	intptr_t Function::ComputeClosureHash() const {
	ASSERT(IsClosureFunction());
	const Class& cls = Class::Handle(Owner());
	uintptr_t result = String::Handle(name()).Hash();
	result += String::Handle(InternalSignature()).Hash();
	result += String::Handle(cls.Name()).Hash();
	return result;
	}

@rmacnak-google says this is slow and could be implemented to be a lot faster. At the very least, it seems strange to me that its getting strings, computing their hash codes, and then adding them.

/cc @a-siva @goderbauer

[dnfield]

FWIW: The use case here is going to be changed in the framework code to use a more optimized ChangeNotifier implementation that is backed by a List rather than Set. However, I wonder if part of the optimization path there was working around this issue with hashing.

[rmacnak-google]

Also the general case identity hash could be rewritten to not need a runtime call.

[mraleph]

The suspect there is InternalSignature - the rest are just combining precomputed hashes and should be relatively quick. Though it is indeed unfortunate that you need to go to runtime at all to compute this hash the first time for each newly created closure.

/cc @alexmarkov who looked at closure hashes before in https://codereview.chromium.org/2983823002/

[dnfield]

Yes, InternalSignature is what bubbles up in the trace (and its call to BaseTextBuffer::addString).

[dnfield]

This ended up being not as trivial to fix in the framework, but also not showing a huge win in real application workloads. It'd still be good to have a fix for it, since I suspect Flutter applications will end up on this path at times.

[mraleph]

I think @rmacnak-google has fixed this in ecdf148 .

[mraleph]

(There is still one runtime crossing for each new closure as I understand it - so it could probably be improved further, but at least we don't format signatures as strings anymore).

[dnfield]

This rolled into flutter/engine in flutter/engine#30327

As of this writing, it has not rolled into framework. However local profiling that drew my attention to this shows marked improvement - it's a little hard for me to precisely quantify given the tooling, but I'm not even really seeing this in CPU profiling where Iw as seeing it before.

[a-siva]

@dnfield Can this issue be closed ?

[dnfield]

It is closed and can stay closed :). Did you mean to post this on a different issue?

[a-siva]

You had a comment that it hadn't rolled into the framework and I was wondering if some issues were found when rolling into the framework. I guess it can remain closed.

[dnfield]

Ahhh haha I tested it manually before it rolled in is all. Sorry for confusion