WIP: Faster GatedRecurrent

[rizar]

Less multiplications in the inner graph should be faster.

Also, I remove the flags switching of the gates. If one wants to research his custom recurrent transitions, they will still need more flexibility.

[bartvm]

Did you benchmark if this is faster than two separate matrix multiplications? I can remember @pbrakel saying that in Theano the benefit from performing a single GEMM is actually cancelled out by the slicing operations needed.

[rizar]

Yes I did, it is faster. I will make a nice benchmark for the record tomorrow (all GPUs are busy), so far my rough estimate is that you can win 20% on 250 GRU units with that.

[dmitriy-serdyuk]

Can you test a backward path as well? Because Theano may need to reallocate memory for a merge op (which is the gradient for split).

[rizar]

20% estimate is for both forward and backward. I got 10% speedup for a big model, for which GRU takes roughly half of the time.

But will take actual profiles tomorrow.

[rizar]

An Easter egg: it becomes non-trivial to initial both reset and update matrices orthogonally, since they are one matrix now.

[dmitriy-serdyuk]

And one more thing: it may be more efficient to store a transpose of the weight matrix since it is faster to split by the first dimension. I'm not sure that GPU stores matrices in C order, though.

[dwf]

AFAIK it does store arrays in C order.

[rizar]

Results, columns stand for number of neurons, the units are milliseconds. The batch size is 10.

Depending on the number of neurons, the new implementation can be quite a bit better, And apparently, our LSTM implementation is slow.

	100	250	1000	2000
SimpleRecurrent	8	14	68	96
new GatedRecurrent	16	31	152	270
old GatedRecurrent	16	41	199	280
LSTM	37	89	303	1032

The code can be found at https://github.com/rizar/blocks-benchmarks

[bartvm]

Cool. Did you try @dmitriy-serdyuk's suggestion to see if it made a difference? (Also, you need a rebase.)

[rizar]

I am not sure I understand it, to be honest. It is not the weight matrix, but the dot product results that are split. I guess I can switch the order to terms, in the line 531 and transpose the subtensors to get reset_values and update_values, but for me it is not obvious that is will be faster that way.

[bartvm]

I guess the idea would be to check if this makes a difference

gate_values = self.gate_activation.apply(self.state_to_gates.dot(states.T) + gate_inputs.T)
update_values = gate_values[:self.dim]
reset_values = gate_values[self.dim:]

[rizar]

To make it work I had to a few more transpositions, and as a result it got worse:

gate_values = self.gate_activation.apply(              
    self.state_to_gates.dot(states.T) + gate_inputs.T) 
update_values = gate_values[:self.dim].T               
reset_values = gate_values[self.dim:].T                

	100	250	1000	2000
this GatedRecurrent	88	104	230	388

[bartvm]

I guess it's because states needs to be transposed, makes sense.

[rizar]

Right, and I guess the following is the reason why LSTM is slow:

def slice_last(x, no):
    return x.T[no*self.dim: (no+1)*self.dim].T

As soon as tests pass, I think this PR will be ready to merge.

[rizar]

I changed initialization procedure so that it was identical to the earlier version of GatedRecurrent.

[pbrakel]

Removing the slicing from the LSTM has been on our todo-list for a while now because it is indeed the most likely cause of terrible speed. Of course it has a lot more parameters than the other models but it shouldn't be that much slower. I'll see if I can find some time to look at it today.

[rizar]

@pbrakel , let me make a blind guess, I think it is not slicing, but transposition that hurts so much. You can use my benchmarking script, by the way.

[pbrakel]

@rizar that script would be very helpful. I think the slicing might actually not be the main reason for slow simulation time but mainly for a slowing down of the gradient computations.

[rizar]

@pbrakel , here it is: https://github.com/rizar/blocks-benchmarks/blob/master/benchmark-rnns.py

[pbrakel]

@rizar Thanks, Dima!

[rizar]

Rebased to account for #600

Also, fixes #399

[rizar]

@bartvm , do you know why Scrutinizer was not even run on this PR?

[bartvm]

It says "Status: Ignored; this pull-request is not mergeable." because GitHub sent the following payload:

        "mergeable": null,
        "mergeable_state": "unknown",

Which is odd... Because mergeable is supposed to be a Boolean value.

[rizar]

Scrutinizer says OK. Merging?