GH-127953: Make line number lookup O(1) regardless of the size of the code object #128350
Conversation
markshannon
added
needs backport to 3.12
|
Benchmarking shows no significant change in performance. The coverage benchmark shows a small speedup, but it is probably noise. |
| return COMPUTED_LINE; | ||
| int delta = line - code->co_firstlineno; | ||
| assert(delta > NO_LINE); | ||
| return delta; |
There was a problem hiding this comment.
Looks like line_delta is actually an offset from the start now.
| get_line_delta(_PyCoLineInstrumentationData *line_data, int index) | ||
| { | ||
| uint8_t *ptr = &line_data->data[index*line_data->bytes_per_entry+1]; | ||
| uint32_t value = *ptr; |
There was a problem hiding this comment.
Could assert here that bytes_per_entry is within range.
There was a problem hiding this comment.
We can't easily assert anything here, as we don't know the max line number without traversing the entire locations table.
I've added an assert to set_line_delta instead.
There was a problem hiding this comment.
I meant that bytes_per_entry is between 1 and 4.
There was a problem hiding this comment.
(bytes_per_entry is between 2 and 5 because it includes the original opcode)
|
I’m taking a vacation now and I’ll take a look once I’m back! |
| @@ -38,8 +38,8 @@ typedef struct { | |||
| Line instrumentation creates an array of | |||
| these. One entry per code unit.*/ | |||
There was a problem hiding this comment.
Does One entry per code unit still hold for the new struct?
| assert(line_delta >= NO_LINE); | ||
| uint32_t adjusted = line_delta - NO_LINE; | ||
| uint8_t *ptr = &line_data->data[index*line_data->bytes_per_entry+1]; | ||
| assert(adjusted < (1ULL << (line_data->bytes_per_entry*8))); |
There was a problem hiding this comment.
This assertion should minus the opcode byte right? assert(adjusted < (1ULL << ((line_data->bytes_per_entry - 1)*8)))
| if (line_data->bytes_per_entry > 3) { | ||
| if (line_data->bytes_per_entry > 4) { | ||
| assert(line_data->bytes_per_entry == 5); | ||
| *ptr = adjusted >> 24; |
There was a problem hiding this comment.
I think we should do a little endian here so we can put this in a loop:
for (int idx = 1; idx < line_data->bytes_per_entry; idx++) {
*ptr = adjusted & 0xff;
ptr++;
adjusted >>= 8;
};Same thing for decoding.
| else { | ||
| bytes_per_entry = 5; | ||
| } | ||
| code->_co_monitoring->lines = PyMem_Malloc(1 + code_len *bytes_per_entry); |
There was a problem hiding this comment.
| code->_co_monitoring->lines = PyMem_Malloc(1 + code_len *bytes_per_entry); | |
| code->_co_monitoring->lines = PyMem_Malloc(1 + code_len * bytes_per_entry); |
Changes the line data array from 2 bytes per instruction to 2-5 bytes per instruction depending on the line length of the code object, changing the size of the line delta from 1 byte to 1-4 bytes as needed.
Most code objects are fewer than 250 lines, and still use 1 byte for line delta per instruction.
Code objects up to ~65k lines need 2 bytes per instruction.
3 bytes are needed for code objects up to ~16M lines long.
4 bytes are needed for code objects up to the maximum of ~1 billion lines.
I doubt 4 bytes will ever be needed, but it is supported.
This PR also fixes the
INSTRUMENT_DEBUGmode which was broken in 2e95c5bThe time taken to execute the example given in #127953 (comment) is about the same as 3.10 or 3.11. Maybe a bit faster, but I didn't benchmark it rigorously.
Modifying the script to generate a file of 100k lines, this PR is a bit faster than 3.11.
At 1M lines, this PR is a bit slower than 3.11.