Here is a patch which restores (removed at last moment before 3.4beta1 release) frame headers optimization for the pickle protocol 4. Frame header now saved as a part of previous frame. This decreases the number of unbuffered reads per frame from 3 to 1.

Hmm... I thought your patch was only an optimization, but if you're overlapping frames (by adding 9 to the frame size), it becomes a protocol change.

I personally am against changing the PEP at this point, especially for what looks like a rather minor win.

Frames are not overlapped. And no one opcode straddles frame boundaries. When an implementation supports optional frames, it should support optimized frames as well.

All tests are passed with this optimization except test_optional_frames which hacks pickled data to remove some frame opcodes. This test relied on implementation details.

Bad wording perhaps, but:

+                    if not final:
+                        n += 9  # next frame header
                     write = self.file_write
                     write(FRAME)
                     write(pack("<Q", n))

does change how the frame length is calculated and emitted in the pickle stream.

This is not compliant with how the PEP defines it (the frame size doesn't include the header of the next frame):
http://www.python.org/dev/peps/pep-3154/#framing

> All tests are passed with this optimization

Well, perhaps there are not enough tests :-) But the protocol is standardized so that other people can implement it. The reference implementation can't do something different than the PEP does.

If it's an optimizatio, can I see some benchmarks numbers? :-)

I would be interested of benchmarks pickle 3 vs pickle 4.

> Bad wording perhaps, but:
> 
> +                    if not final:
> +                        n += 9  # next frame header
>                      write = self.file_write
>                      write(FRAME)
>                      write(pack("<Q", n))
> 
> does change how the frame length is calculated and emitted in the pickle
> stream.

Of course (as any optimizer). It produces more optimal pickled data which can 
be parsed by existing unpicklers.

> This is not compliant with how the PEP defines it (the frame size doesn't
> include the header of the next frame):
> http://www.python.org/dev/peps/pep-3154/#framing

"How the pickler decides to partition the pickle stream into frames is an 
implementation detail."

> > All tests are passed with this optimization
> 
> Well, perhaps there are not enough tests :-) But the protocol is
> standardized so that other people can implement it. The reference
> implementation can't do something different than the PEP does.

Could you write tests which exposes behavior difference without sticking 
implementation details?

> If it's an optimizatio, can I see some benchmarks numbers? :-)

First create two files. Run unpatched Python:

./python -c "import pickle, lzma; data = [bytes([i])*2**16 for i in 
range(256)]; with open('test.pickle4', 'wb'): pickle.dump(data, f, 4)"

Then run the same with patched Python for 'test.pickle4opt'.

Now benchmark loading.

$ ./python -m timeit "import pickle;"  "with open('test.pickle4', 'rb', 
buffering=0) as f: pickle.load(f)"
10 loops, best of 3: 52.9 msec per loop

$ ./python -m timeit "import pickle;"  "with open('test.pickle4opt', 'rb', 
buffering=0) as f: pickle.load(f)"
10 loops, best of 3: 48.9 msec per loop

The difference is about 5%. On faster computers with slower files (sockets?) it 
should be larger.

> On faster computers with slower files (sockets?) it should be larger.

If your patch avoids unbuffered reads, you can test using these commands before your benchmark:

    sync; echo 3 > /proc/sys/vm/drop_caches

And use one large load() instead of running it in a loop. Or call these commands in the loop.

> > This is not compliant with how the PEP defines it (the frame size doesn't
> > include the header of the next frame):
> > http://www.python.org/dev/peps/pep-3154/#framing
> 
> "How the pickler decides to partition the pickle stream into frames is an 
> implementation detail."

Of course, but it still has to respect the frame structure shown in the
ASCII art drawing.

> Could you write tests which exposes behavior difference without sticking 
> implementation details?

That's a good idea. At least I'll open an issue for it.

> If your patch avoids unbuffered reads, you can test using these commands
> before your benchmark:
> 
>     sync; echo 3 > /proc/sys/vm/drop_caches

Thank you. But starting Python needs a lot of I/O. This causes very unstable 
results for this test data.

> This causes very unstable results for this test data.

So try os.system("sync; echo 3 > /proc/sys/vm/drop_caches") in your timeit benchmark.

> Of course, but it still has to respect the frame 
structure shown in the
> ASCII art drawing.

I think the ASCII art drawing is just inaccurate. It 
forbids optional framing (tested in 
test_optional_frames).

Serhiy, I am not able to reproduce your results. I don't get any significant performance improvements with your patch.

22:34:03 [ ~/PythonDev/cpython-baseline ]$ ./python.exe -m timeit "import pickle" "with open('test.pickle4', 'rb', buffering=0) as f: pickle.load(f)"
100 loops, best of 3: 9.26 msec per loop

22:36:13 [ ~/PythonDev/cpython ]$ ./python.exe -m timeit "import pickle" "with open('test.pickle4', 'rb', buffering=0) as f: pickle.load(f)"
100 loops, best of 3: 9.28 msec per loop

I wrote a benchmark to simulate slow reads. But again, there is no performance difference with the patch.

22:24:56 [ ~/PythonDev/benchmarks ]$ ./perf.py -v -b unpickle_file ../cpython-baseline/python.exe ../cpython/python.exe
### unpickle_file ###
Min: 1.103715 -> 1.103666: 1.00x faster
Avg: 1.158279 -> 1.146820: 1.01x faster
Not significant
Stddev: 0.04127 -> 0.03273: 1.2608x smaller

If you want a slow file object, you could benchmark with a GzipFile or similar.

> 22:36:13 [ ~/PythonDev/cpython ]$ ./python.exe -m timeit "import 
pickle"
> "with open('test.pickle4', 'rb', buffering=0) as f: pickle.load(f)" 100
> loops, best of 3: 9.28 msec per loop

Did you use the same test file or different files (generated by patched 
and unpatched pickler)? Try to run this command several times and take a 
minimum.

> I wrote a benchmark to simulate slow reads. But again, there is no
> performance difference with the patch.

Test data are too small, they all less than frame size.

> Test data are too small, they all less than frame size.

Ah, good point! It seems your patch helps when the reads are *very* slow and buffering is disabled.

### unpickle_file ###
Min: 1.125320 -> 0.663367: 1.70x faster
Avg: 1.237206 -> 0.701303: 1.76x faster
Significant (t=30.77)
Stddev: 0.12031 -> 0.02634: 4.5682x smaller

That certainly is a nice improvement though that seems to be fairly narrow use case. With more normal read times, the improvement diminishes greatly:

### unpickle_file ###
Min: 0.494841 -> 0.466837: 1.06x faster
Avg: 0.540923 -> 0.511165: 1.06x faster
Significant (t=4.10)
Stddev: 0.03740 -> 0.03510: 1.0657x smaller

While the speedup may be nice, I still don't think this optimization complies with the protocol definition in the PEP, so I would like to reject this patch.

Yeah, let's close this. It is much simpler to just double the frame size target if the extra reads ever become a performance issue.