Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Speedup from switch to +=

github.com

Post reply on HN
Print view View original

61–70 of 85 posts

Re: Speedup from switch to +=

#61

Post by staticassertion »

mgraczyk wrote: post #59
Arnavion wrote: post #51

Earlier quoted context omitted.

staticassertion's point is that the current code's usage of `+=` mutates the x that was passed in by the caller, and their suggestion is to copy x into a function local before mutating it, which is similar to how the original `+` code also worked on a function local x (the result of `attn1() + x`).

That's not the problem though. The problem is that the += operations mutate x in place, but the right hand side reads from x. There is no copy you could insert like that to fix this. You would have to do the following, for example Instead of x = op(x) + x -> x += op(x) Do x_copy = copy(x) x += op(x_copy) If you do x_copy += op(x_copy) Then you are still mutating x_copy while op() reads it. EDIT: I also don't think co…

Ah, ok, I assumed the issue was happening outside of the function. If the issue is actually those intermediaries being mutated, bummer.

As for copy vs deepcopy, like I said, I have no idea what the type is so I don't know that deepcopy is necessary or not.

Re: Speedup from switch to +=

#62

Post by nodja »

I see lots of people answering why it's faster, but not many saying why the engineers chose the slower version.

As everyone said, this is more performant because x is being modified in place, the reason this was not done in place is because you can't train a neural network if an instruction is being done in place. During training a network goes literally through all operations that were done and see how well they performed so they can be adjusted using a secondary value called a gradient, this is done during the backwards pass. If you replace something in place you're essentially overwriting the input values that were passed to that function, and by extension, the output values of the function called before, essentially breaking the network chain, unless you also copy the inputs together with the gradients, which would cause an even worse performance hit and be a memory hog.

The breakage bug later in the issue is proof of this, when sampling to generate an image only the forward pass is done on the network, but textual inversion requires you to train the network and therefore do the backwards pass, triggering the error since the dependency graph is broken. I should also note that technically the add operation should be safe to do in place as it's reversible, but I'm not a pytorch expert so I'm not sure exactly what's going on in there.

Re: Speedup from switch to +=

#63

Post by maweki »

mgraczyk wrote: post #50
desmond373 wrote: post #49

Earlier quoted context omitted.

Might be platform dependent whether that first line counts as a mutate or not, seeing as it can be converted to not do anything in some cases.

All of the lines that have += mutate x in place

Unless x is immutable, in which case the semantics of both variants agree.

Re: Speedup from switch to +=

#65

Post by umvi »

nodja wrote: post #62

I see lots of people answering why it's faster, but not many saying why the engineers chose the slower version. As everyone said, this is more performant because x is being modified in place, the reason this was not done in place is because you can't train a neural network if an instruction is being done in place. During training a network goes literally through all operations that were done and see how well they per…

See, this is a great example of where a comment needed to be added, but wasn't.

If the engineers that originally implemented the function intentionally chose the slower version, a quick comment as to why would have prevented this from happening in the first place.

Re: Speedup from switch to +=

#66

Post by fragmede »

WirelessGigabit wrote: post #41
thomasahle wrote: post #37

Earlier quoted context omitted.

In PyTorch `x = y + x` is actually semantically different from `x += y`, so you can't easily make the switch with a compiler. The difference is that `x += y` modifies `x` inplace, where `x = x + y` creates a new object. In other words, if anybody had a reference to `x` before the update, the "optimized" code would break things.

Compiler could use a pointer to pointer. I guess this is the kind of this stuff that drew me to Rust. This kind of behavior gives me the creeps. Just like Ruby’s conventions.

Rust has the same behavior. https://news.ycombinator.com/item?id=32805756

Re: Speedup from switch to +=

#67

Post by hbogert »

umvi wrote: post #65
nodja wrote: post #62

I see lots of people answering why it's faster, but not many saying why the engineers chose the slower version. As everyone said, this is more performant because x is being modified in place, the reason this was not done in place is because you can't train a neural network if an instruction is being done in place. During training a network goes literally through all operations that were done and see how well they per…

See, this is a great example of where a comment needed to be added, but wasn't. If the engineers that originally implemented the function intentionally chose the slower version, a quick comment as to why would have prevented this from happening in the first place.

Will you be my colleague please?

This is idd the time to place a comment, yet so many people don't do that.

Re: Speedup from switch to +=

#68

Post by kjeetgill »

dahfizz wrote: post #23
bee_rider wrote: post #11

Earlier quoted context omitted.

The += operator is almost certainly calling some method on sends out the real work to some tuned hardware-specific framework written in a fast language.

So python is marshalling data to and from an ffi in the fast path? That sounds even worse

I think you mean critical path (which is ironically usually the slowest path). A fast path is usually a hardcoded shortcut you can take for select cases.

Re: Speedup from switch to +=

#70

Post by nodja »

umvi wrote: post #65
nodja wrote: post #62

I see lots of people answering why it's faster, but not many saying why the engineers chose the slower version. As everyone said, this is more performant because x is being modified in place, the reason this was not done in place is because you can't train a neural network if an instruction is being done in place. During training a network goes literally through all operations that were done and see how well they per…

See, this is a great example of where a comment needed to be added, but wasn't. If the engineers that originally implemented the function intentionally chose the slower version, a quick comment as to why would have prevented this from happening in the first place.

This is common knowledge, so common that someone that hasn't coded anything besides some basic linear regression model like me knows about it. It's like commenting on why you'd put parenthesis in some formula, it's just gonna say "parenthesis here because this operation takes priority", similarly in a pytorch model, if it was done by those standards the code would be filled with "operation not done in place because it would break the network graph". You're more likely to encounter the opposite comment, "doing this operation in-place because it'll be discarded later" or something along those lines.

One of the first things you're taught when learning pytorch is that you're not coding in python, but actually creating a network graph that is loaded and executed on a GPU. Other common sense things is knowing that you shouldn't use stuff that is in the stdlib or in numpy and use torch.* variants instead, not doing so will incur either undefined behavior, cause massive memory copies between the CPU and GPU or most likely, error out at runtime.

Note that this is a repo that is forked from the official repo, it's a community repo focused on inference and thus doesn't care about training so it has completely different considerations than the original code.

Post reply on HN