Arne Goedeke wrote:

a += b is a shorthand for a = a + b, so the refs==1 check is the only correct condition for when to use the `+= lfun. The `+= lfun therefore is _not_ operator overloading for +=, but rather an optimization for when a has only one reference.

Yes, that seems to describe what happens. That is not what someone would expect that implements an `+= lfun on an object.

The question now remains: - Do we agree this is a bug? - Or do we document that this is a feature? - Or maybe we already documented it as a feature and I just didn't see it.

Tobias S. Josefowitz wrote:

Then they should have read the docs: http://pike.lysator.liu.se/generated/manual/modref/ex/lfun_3A_3A/_backtick_a...

Ok, it was option three then, I guess. I apologise for the noise. Let the record show though that I consider it to be a rather messy way to handle this :-).

Ah. So that is why nobody is replying to the mails I send.

It would appear that the KOM<->email bridge is not working correctly, so the last few replies I have written have not, actually, been sent.

First, on this topic:

Object `+= lfuns are not called when they should have been.

Well. That depends on how you define "should have been.". :)

`+= has never been called except when there is only one reference to the object, which is why it can be somewhat confusing. It does allow optimizations since the value can be modified destructively, however.

And, really, X += Y only implies that the variable X will have Y added to it, it is not the same as whatever X pointing to getting Y added.

(this is much the same as if there had been a `=, assignment overloading)

Consider:

| mapping m = ([]); | object o = 1003443894984389348989434389; | multiset z = (<>); | array q = ({}); | string b = "";

In which cases should using += change not only the variable but also what it points to?

I think this would be somewhat confusing, as an example:

| object a = 18042893489438948390; | object b = 182389983498439843983498; | object c = b;

a += b;

// afterwards c == b, while I would expect (a-b == c) to be true.

As for how the += optimization is supposed to work:

• For one, the object might have an extra instance on the stack, in which case the minimum number of references becomes 2 already.

Well, this should be mostly fixed by the fact that a = a + X; should be handled differently than other additions, the lvalue (the variable being assigned to and read from) is actually cleared before the addition happens.

The code that is supposed to ensure this is the case lives in line 1337 in las.c (nice..).

The non-existence of +=/-= etc as actual opcpdes is done in treeopt.in, but note that even if those rules are removed there is actually no += opcode in the generated code, and there never has been to my knowledge, the conversion was done in docode.c previously however, which led to a lot of code duplication.

• And for another, the object might have multiple instances which all refer to the same object; which *implies* that updating one of those using += should modify all of them.

Well. Not really, in pike. Adding things to a variable only ever change the variable, not the thing it points to.

IOBuffer talk:

The actual reason I beefed up Buffer a bit lately is *because* I need to do some protocol decoding of a byte stream.

Now I see IOBuffer arrive. In order to avoid code bloat, wouldn't it be a better idea to integrate the functionality of IOBuffer into Buffer and just keep one?

Sorry about the timing, I have had IOBuffer on the way for some time (I am still wondering where to put it, however, that has, believe it or not, been a blocker for me. Perhaps Stdio.Buffer? I will create a buffered stream that reads to and writes from said object, without creating pike strings)

Unfortunately it is not possible to make String.Buffer even close to as efficient as long as it uses a stringbuilder. And not using a stringbuilder slows some things down (sprintf comes to mind) and makes others more or less impossible (wide strings) without excessive code duplication.

The whole reason for IOBuffer is that it uses pointer arithmetics on guaranteed 8bit strings to be fast at both reading from the beginning and writing to the end at the same time (I am, by the way, considering converting it to be a circular buffer to avoid the one memmove it does at times), and it is also efficient at creating sub-buffers.

The fact that it is guaranteed to only contain 8bit characters helps a lot too.

Or is the performance difference so strikingly great that this sounds like a bad idea?

As things stands now, yes.

Things like subbuffers is unfortunately actually impossible when using a stringbuilder.

I guess I might outline the plan for IOBuffer some more (I actually did this during the last pike conference, but it has been a while. :))

o Add support for reading to and writing from file objects to it. Either add support in Stdio.File (also add System.Memory + String.Buffer?) or do it the other way around (that way lies madness, however, see also: Shuffler)

The main goal here is to do one copy less and avoid the pike string creation

o Add support for System.Memory & String.Buffer to add()

o Add support for reading other things than "binary holerith" and integers + line + word + json object + encode_value?

o Add support for "throw mode".

It is rather useful to be able to change what happens when you try to read data that is not in the buffer.

| void read_callback() | { | inbuffer->add( new_data ); | // Read next packet | while( Buffer packet = inbuffer->read_hbuffer(2) ) | { | packet->set_error_mode(Buffer.THROW_ERROR); | if( mixed e = catch(handle_packet( packet ) ) ) | if( e->buffer_error ) | protocol_error(); // illegal data in packet | else | throw(e); // the other code did something bad | } | }

The handle_packet function then just assumes the packet is well formed, and reads without checking if the read succeed (since it will, or an error will be thrown).

This code snipplet also demonstrates why the subbuffers are handy, read_hbuffer does not actually copy any data, the returned buffer just keeps a reference to the data in the original buffer.

-- mail #2

I've been tracking IOBuffer extensions back to String.Buffer, I'll present what I have shortly.

I suspect (but benchmarks will have to tell) that the String.Buffer implementation is not significantly slower than the current IOBuffer one (whilst supporting the full range of character widths).

Well. You have some minor optimizations to do:

| int perf(object buffer) | { | buffer->add("11"); | for(int i=0;i<10000; i++ ) | { | int l; | if( buffer->cut ) | { | l = (buffer[0]<<8) | buffer[1]; | buffer->cut(0,2,1); | } | else | l = buffer->read_int(2); | buffer->add(random_string(l)); | return sizeof(buffer); | } | }

perf( String.Buffer() );

Result 2: 325250971 Compilation: 624ns, Execution: 144.86s

perf( Stdio.IOBuffer() );

Result 3: 328787331 Compilation: 639ns, Execution: 194.06ms

(note that the length differs due to random_string)

However, reviewing the IOBuffer interface, I wonder about the following issues:

• Isn't it prudent to drop set_error_mode() and simply implement this functionality (the throw()) using a custom range_error() override?

Well. That would work, yes, I just simply did not remove the old version of throwing errors since it would most often be used using the simply buff->set_error_mode(1) when doing sub-parsing as I showed in the documentation for set_error_mode.

The need to do rather complex sub-classing for that common usecase seemed somewhat pointless.

• Why insist on lock()ing the buffer when subbuffers are active? Couldn't the code figure out by itself when a subbuffer exists and then decide on-demand and automatically when a copy needs to be made to transparently support the desired operation?

Not really, since the subbuffer only contains a pointer directly into the memory area of the main buffer, if the main buffer changes that using realloc or malloc it would be invalid, this could of course be fixed by adding a list of subbuffers to the main buffer, but then you run into issues with refcounting and such. Since the usecase where you have a subbuffer active and want to modify the main buffer is rather uncommon I thought it was OK that you have to call trim() on the subbuffer to do that.

Why not return IOBuffers practically everywhere, and then let the caller decide when and if to cast them to a string? It gets rid of excessive method diversification due to there needing to be a string and a buffer returning one. Returning a buffer is cheap, it doesn't copy the content.

Well, there is about a factor of 3 performance difference:

string perf2(object b) { while( b->read(1) ); } string perf3(object b) { while( b->read_buffer(1) ); }

perf2(Stdio.IOBuffer(mb100));

Result 5: 0 Compilation: 664ns, Execution: 92.19ms

perf3(Stdio.IOBuffer(mb100));

Result 6: 0 Compilation: 680ns, Execution: 173.68ms

Since that does not include the cast, which should be about as fast as the first read, it becomes about 3x slower.

And most of the time you actually want the string version, not the buffer version.

-- mail #3

Erm... We are *in* a Buffer object, so by definition we have one. So returning a readonly-copy with zero-copy effort is easy. It basically delays the creation of the shared string as long as possible.

Not really, we are in /a/ buffer object, not the subsection of it that should be returned. You have to create a new one to return a subsection. read_buffer is about as fast as it gets, it does the minimal amount of work.

As long as one is doing string operations (adding/substracting/matching) Buffer objects are better. Once done with that, the final "result" can/should be a shared string.

Nothing that adds data to the buffer returns a string in the current buffer code.

The only thing that returns a string is if you call read() or read_hstring() on it.

Once done with that, the final "result" can/should be a shared string.

An additional comment: By definition you are almost never actually "done" with a IOBuffer.

They are designed to be input and output buffers for IO.

-- mail #4

An additional comment: By definition you are almost never actually "done" with a IOBuffer.

They are designed to be input and output buffers for IO.

And now the basic support is there to use them for Stdio.File objects.

Stdio.File now has a new nonblocking mode: Buffered I/O

In this mode the file object maintains two buffers, one for input and one for output.

The read callback will get the buffer as the second argument, and data that the user does not read from that buffer is kept until the next time data arrives from the file (this means you do not have to do your own buffering of input)

The output buffer is, unsurprisingly, used to output data from.

This has at least three somewhat convenient effects:

o The write callback will now receive that buffer as a second argument. You just add data to it to write it.

o Adding data to the buffer when /not/ in the write callback will still trigger sending of data if no write callback is pending.

o Your write callback will not be called until the buffer is actually empty.

An extremely small demo:

| void null() {} | | int main() | { | Stdio.IOBuffer output = Stdio.IOBuffer(); | Stdio.File fd = Stdio.File(0); | | fd->set_buffer_mode( 0, output ); | | fd->set_nonblocking( Function.uncurry(output->add), null, null ); | | return -1; | }

This will case all data received on stdio to be eched to .. stdin using buffered output.

Not the most useful application, but it does show how easy it is. Buffered mode in general is mainly useful because it removes the need for you to handle the buffers manually in your code.

Currently read() and write() are not in any way modified by having buffered output enabled, if you interact directly with the file object it will bypass the buffer. I am unsure if this is a good idea or not.

-- Per Hedbor

Generally speaking, buffers help enormously when reading and parsing from bytestreams into pike, but they do not help all that much when writing to bytestreams. The writev() system call handles the fragmented writes quite nicely (see the pgsql driver) and more efficient than an IOBuffer which would copy the data at least once.

That is only true if you do not include the overhead you get from making the strings. It is also only even close to true if you can always write the whole buffer.

The very common case where you have nonblocking output means you always have to chop off data from the strings, if you have them, and keep track of the array members when you use writev.

The actual gain when I switched the communication pipe in one of subprocesses in the storage system we have at Opera to BinaryBuffer, which is basically a predecessor to IOBuffer, when communicating encode_value:ed values in arrays was about 98% speed gain in the buffer handling. It went from 80% to less than 2% of the overall CPU time.

Meanwhile the number of lines needed to do it went from 20 to 10. And that buffer did not have write_to

Basically:

Before, pseudo code of course: array outbuf; output(mixed value) { outbut += ({ sprintf("%4H", encode_value(value)) });}

write_callback() { int num = fd->write(outbuf[..100]); // if 100 is left out this is... rather slow.. for large buffers // remove entries from outbuf until sizeof(outbuf[0]) < num.. // this was of course done in one step outbuf[0] = outbuf[0][num..]; // << 99.99% of the CPU time goes here. }

After: BinaryBuffer outbuf; output: outbuf->add_hstring(encode_value(value),4);

write_callback() { string chunk = outbuf->read(4096); int num = fd->write( outbuf ); outbuf->unread( 4096-num ); }

...

Things like subbuffers is unfortunately actually impossible when using a stringbuilder.

Well, it's not, really. Unless I made some rather striking mistakes.

How do you add the trailing 0, and how do you avoid the fact that the string_builder_x functions can at any time reallocate the 's' member to point to another memory location, making the sub-object totally invalid, and how do you avoid the fact that the str member is actually _part of_ the struct pike-string structure, so you at least temporarily have to overwrite the data in the main buffer?

...

...

perf( String.Buffer() );

Result 2: 325250971 Compilation: 624ns, Execution: 144.86s

Erm, how do you get this nice looking timings? Do I have to turn on a profiling flag on Hilfe somewhere?

set format bench

(did you note the factor of 1k difference in speed in the test?

Benefits of the new String.Buffer implementation: - Close to drop-in replacement for IOBuffer. - Performance hovers around that of IOBuffer's. - Support all native Pike character widths. - Backward compatible with the old String.Buffer. - Creating a String.Buffer object from a shared string does *not* incur a single copy (and is thus very lightweight). - A String.Buffer initialised from a shared string allows one to access the shared string in parts without copying the data at all. - Supports System.Memory objects just like IOBuffer. - No need to account for locks on subbuffers; String.Buffer resolves those automatically and on demand. - Explicit copying of the buffer data is not supported anymore, the implicit copy-on-demand mechanism is more efficient.

Stephen R. van den Berg wrote:

...

...

...

Things like subbuffers is unfortunately actually impossible when using a stringbuilder.

...

...

Well, it's not, really. Unless I made some rather striking mistakes.

Behold, the source code of String.Buffer NG has been checked in.

...

How do you add the trailing 0, and how do you avoid the fact that the

The trailing \0 is only required for shared strings, I streamlined the string_builder code dealing with that in a separate patch.

...

string_builder_x functions can at any time reallocate the 's' member to point to another memory location, making the sub-object totally invalid, and how do you avoid the fact that the str member is actually _part of_ the struct pike-string structure, so you at least temporarily have to overwrite the data in the main buffer?

This is solved using refcounts and copying the stringbuffer as soon as we want to modify one that has more than one ref.

With regard to benchmarks and speed. I'd say String.Buffer now rivals and possibly sometimes exceeds the performance of IOBuffer. Case in point:

int perf(object buffer) { buffer->add("11"); for(int i=0;i<100000; i++ ) { int l; if( buffer->cut ) { l = (buffer[0]<<8) | buffer[1]; buffer->cut(0,2,1); } else l = buffer->read_int(2); buffer->add(random_string(l)); } return sizeof(buffer); }

perf( String.Buffer() );

Result 37: 3270303901 Compilation: 936ns, Execution: 5.72s

perf( Stdio.IOBuffer() );

Result 35: 3274269166 Compilation: 941ns, Execution: 5.60s

But, String.Buffer supports close to all methods of IOBuffer, so a fairer test would be:

int perf(object buffer) { buffer->add("11"); for(int i=0;i<100000; i++ ) { int l; l = buffer->read_int(2); buffer->add(random_string(l)); } return sizeof(buffer); }

perf( String.Buffer() );

Result 44: 3271319355 Compilation: 949ns, Execution: 5.69s

perf( Stdio.IOBuffer() );

Result 41: 3278225479 Compilation: 942ns, Execution: 5.59s

Looks close enough. One should compare program-code-size too, I didn't bother to do that yet because it is part of builtin.cmod. I'd wager that my String.Buffer implementation has a smaller code footprint than the current IOBuffer.

Comments, criticism, patches are welcome, of course.

Proposed change:

Change the Buffer add_hstring( string|Buffer|object str, int size_size ) method into Buffer add_hstring( int size_size, string|object|int|array(string|object|int) ... str )

Per Hedbor wrote:

You can fix most of the very slow functions, but getting better than about 50% performance is rather hard when you have to check the size-shift when extracting and adding chars. The cpp() function became about 40% slower after my rewrite that changed it to use PCHARP everywhere, as an example.

Ok, can't argue with that.

Also, the fact that String.Buffer can contain non-8bit characters is directly /harmful/ when doing an IOBuffer. So the extra overhead is not really there for any good reason.

Well, I wouldn't say harmful, non-obvious perhaps. The system calls ultimately do the 8-bit check. Don't get me wrong: I come from an 8-bit world, I just learned to live with the fact that in Pike you have to deal with more, usually.

Why not simply keep String.Buffer as a simple string-adding-buffer, and let the more complex stuff be done in other code, as before? IOBuffer is really only a more efficient version of ADT.struct. Why is it so dangerous to have it around? I find the additions to String.Buffer to be significantly less beautiful, if that is a way to put it. There was not really any shared funcitonality between String.Buffer and Stdio.IOBuffer except one before: Adding strings to a buffer that is not otherwise used until "done".

Well, ok, but then I suggest we dumb down String.Buffer again to what it was in 7.8, and get rid of the addat() as well. Since the addat() use case becomes rather narrow considering the existence of IOBuffer. Or am I missing something as to its usefulness in String.Buffer?

It is somewhat esoteric to use add_int(..) add_short() etc when the string contains widechars.

Correct :-). I did have a slightly tingly feeling when considering what the semantics were supposed to be in that case.

It does not really make sense (incidentally, your JSON parser assumes utf8 which is hardly true for a string, those are supposed to be unicode)

The JSON parser is only useful if unleashed on 8-bit strings, of course.

Also, how do you handle the 2G+ buffer case? I can see no code for that (yes, I have hit 2G+ buffer sizes, which is why IOBuffer is very carefully using size_t everywhere).

Indeed. Didn't want to fiddle with that until we decide if IOBuffer is dropped or not.

And as for codesize, String.Buffer, not counting any stringbuilder code, compiled using clang -Ofast on my mac, weighs in at 27893 bytes (only counting functions named _Buffer_* in builtin.o, so it does not include the helper functions).

The text segment of buffer.o is 23425. In total. Without using stringbuilder. So, no, not really.

Ok, I'm suprised, but it only shows that overhead creeps in with wide character treatment.

Also, one thing at random I noticed when reading the stralloc.c commits:

• static INLINE int min_magnitude(const p_wchar2 c)

• static INLINE unsigned min_magnitude(const unsigned c) {

• if(c<0) return 2;
• if(c<256) return 0;
• if(c<65536) return 1;
• return 2;

• return c<256 ? 0 : c<65536 ? 1 : 2;

is not really much of a speedup, if any, in fact it reorganized the instructions for me to make it less efficient if c>=0 and <= 255 when using clang (you would need to add some LIKELY to obvious locations). YMMV when the code is included in the location it's called and other compilers are used. Which is sort of the point.

I would have expected the compiler to do the unsigned comparison first and therefore help the common case. I wasn't familiar with the (UN)LIKELY macros; seems like they could help, indeed.

Also, it is generally a good idea to add what is unsigned, we do so every where else. And, pike 32bit chars are actually signed.

You mean add a comment about the forced unsigned comparison?

Another random thing: _encode should probably not empty the buffer. It's a fairly unexpected side-effect.

I believe I copied this from IOBuffer. Maybe I messed it up.

...

Benefits of the new String.Buffer implementation:

• Close to drop-in replacement for IOBuffer.

Yes, but slower, and why?

Well, I'd hoped that the speed difference would only be a few % for the 8-bit case. It turns out that it is not. If it would have been, it would have increased cache-locality by reducing the code footprint of Pike.

...

• A String.Buffer initialised from a shared string allows one to

access the shared string in parts without copying the data at all.

Of course the same is true for IOBuffer. IOBuffer does not even do any copying when you read all data and then later on add a new string. Which String.Buffer does. :)

Yes, well, that was on the todo short-list. But probably not worth fixing if we're downsizing String.Buffer again.

...

• No need to account for locks on subbuffers; String.Buffer resolves

those automatically and on demand.

• Explicit copying of the buffer data is not supported anymore, the

implicit copy-on-demand mechanism is more efficient.

Well, except that the explicit copy is mainly there to make it obvious when you accidentally copy. It was intentional, so this is a drawback.

In String.Buffer it would have been easy to add a performance-check-aid which throws exceptions if buffer content is being copied. I'd actually consider it better design to change IOBuffer to something similar; i.e. automatically copy, but allow an explicitly locked-down buffer which resists automatic copying by throwing exceptions. The idea being that code should "just work", and if you're concerned about performance (because you know the buffers can be big), you lock it down to prevent accidents (usually during development).

Also, am I right in the fact that the whole buffer has to be copied if a subbuffer is live while the main buffer is modified?

Correct. That would have resulted in an exception in the IOBuffer case, my code handles it automagically. I'd prefer the programmer ask for protection instead of enforcing it.

That seems to be set up for rather severe ordo-accidents that will not be detected. Common case:

you have a packet based protocol. You extract a packet as a buffer. You then do a call_out or similar to handle the packet (thread, whatever).

Then new data arrives, and is added to the buffer.

In the String.Buffer case this might copy the buffer, which is never what you want.

Quite. But, consider the alternative with IOBuffer: depending on race conditions and timing, you might not notice the problem for a long time and then it bites you throwing a "locked" exception when the programmer is not around.

Please note that it would be trivially easy to do the same in IOBuffer, just modify the code in io_ensure_unlocked to copy and then unlock, I /intentionally/ did not do so, even when it was suggested here, since it is, in my opinion, much better if you are required to explicitly uncouple the subbuffer, which adds a simple call in one place, than to accidentally copy the whole buffer, which will add extreme overhead for no good reason that will be very hard to find.

:-). I argue the exact opposite, because the tradeoff here ultimately is that a. Unexpected performance problems creep up. b. Unexpected "locked" exceptions creep up due to race conditions.

Given the choice, I prefer a over b anytime. Once I have a, I would "lock down explicitly" and see where it throws, but then it's under programmer control, as opposed to in production and the programmer is not around.

Considering it was added in 2011, I guess its actually in use. I think when removing APIs/features there need to be compelling reasons to do so. The mere fact that addat has narrow use cases (is that even true?) is imho not enough.

On 09/10/14 14:00, Stephen R. van den Berg wrote:

Stephen R. van den Berg wrote:

...

Well, ok, but then I suggest we dumb down String.Buffer again to what it was in 7.8, and get rid of the addat() as well. Since the addat() use case becomes rather narrow considering the existence of IOBuffer. Or am I missing something as to its usefulness in String.Buffer?

Reviewing the functionality, I'd say to keep clear() and sprintf() to String.Buffer, but drop addat() ?

On 09/10/14 14:27, Per Hedbor wrote:

10 sep 2014 kl. 14:23 skrev Arne Goedeke el@laramies.com:

...

Considering it was added in 2011, I guess its actually in use. I think when removing APIs/features there need to be compelling reasons to do so. The mere fact that addat has narrow use cases (is that even true?) is imho not enough.

Well, in some way that is not entirely true: We do try to remove functions that are a bit too narrow in scope.

I have never seen the use case for addat personally. And I did not include it in IOBuffer even though it would be fairly simple. Also, the function name indicates that the data should be added at a specific location, not overwriting anything.

Usually its being deprecated for some time, so people have a chance to fix their code. addat() was never in a stable release, so I guess that doesn't matter..

grubba added it, so I guess roxen uses it somewhere?

The mail gateway is not working, no.

By the way, I think it might still be useful to allow the addition of buffers (and shm/iobuffer?) objects to String.Buffer.

Perhaps we should make a common function that can be used to get len/char* from a given "memory chunk" object.

grubba added it, so I guess roxen uses it somewhere?

Our dev branches still run on 7.8 so there shouldn't be any such dependency.

On Thu, Sep 11, 2014 at 10:58 AM, Arne Goedeke el@laramies.com wrote:

I think there is a couple of features that would be nice to have in IOBuffer. If there is no objections I would go ahead and add them.

1. support for reading/writing signed integers. Not sure about the api,

maybe the current methods should could be renamed read_uint/add_uint instead? The -1 return value also would not work for the read_ methods. The add_int variants actually work fine for signed ints already, due to the cast to unsigned. sprintf()/sscanf() could be used currently but are quite slow.

The returns value is an issue, I guess using UNDEFINED makes sense.

In my "normal" usage I only use the return value for read_buffer and friends however, and then set error mode and just read.

It really simplifies not having to check after each read, I just added the -1 return value since it sort of makes sense. But UNDEFINED should work just as well, really.

And renaming the current ones to read_u* makes sense if we add non-unsigned versions, yes.

And add_int only sort of works, it will break for bignums, and also normal integers if the width > sizeof(INT_TYPE) (the code padds will null characters).

2. read_utf8/add_utf8. its not hard to do, quite common and would save

one string creation and copy.

It would be nice to avoid large amounts of code duplication, however.

3. reading/writing ieee floats. also handled by sprintf/sscanf, but on

most systems would reduce to bswap+memcpy

That might be useful, yes. But is it really _that_ common? I guess it makes since to be complete(ish). :)

4. little endian support? Admittedly the use cases are narrow for

network protocols, but not so much for file formats.

I avoided that since my point of view when initially implementing the buffer was that it was mainly going to be used for network I/O. But it does work for normal files as well, so it might indeed be useful.

arne

On Thu, Sep 11, 2014 at 3:45 PM, Arne Goedeke el@laramies.com wrote:

I would split the current utf8 encoder/decoder into 2 functions, one for calculating the length of the result and one doing the actual encoding/decoding. f_utf8_to_string could then call those two directly, as could IOBuffer. Sounds ok?

Well, yes, as long as it is not slower (and, perhaps most importantly, does not create new strings in the somewhat common case where there is no change when doing the encoding/decoding)

I'd rather have more methods. If I do buf->read_something(a-b); and there is a bug in how a or b was made, I want an exception here, not a wrong and possibly non-failing value out of the read call.

Stephen R. van den Berg wrote:

a. Perform it when called as read_hstring(0);

After reviewing the options, choice a seems most friendly to me.

read_hstring() without an argument would make sense for something like a Hollerith string with a base-128-encoded length though. Don't know if that's a common enough case...

And we really need to address the mail export. I don't even know the address of the mailing list...

If read_hstring() reads Hollerith strings, I'd like read_cstring() for C strings.

Why not simply let the read_int*() functions return all signed values instead?

Because signed values are rather rare in protocols and file formats.

This would:

• Increase the chance of the return value fitting into the native int type.'

Well, sometimes.

• Still allow easy reading of unsigned values (if one insists) by masking the output at the reader end by the appriopriate amount of bits (which is more efficient in Pike than creating a signed from an unsigned).

Not with bignums, actually. And that's the most common use with SSL, as an example. Also, it would mean that each and every time you read a byte or short or whatever you also have to have a matching &.

It would also break hstring reading rather badly, so you would still need the functions to read unsigned numbers internally.

Also, with native integers, as it turns out, it's fairly equal on x86_64 at least, where the integer operations are inlined.

Just add another function to read signed integers.

And as for byteorder, just add a switch to switch byteorder if the number of functions needs to be kept down for some reason.

Well, I admit that the check_avail_mul originally did a better job of figuring out if the specified sizes and numbers exceed the available ptrdiff_t type.

However, all the other changes should be correct and should not introduce buffer overruns. Can you give one counterexample? Because, if I recall correctly, in checking the code I found one error in the range checks in the existing code; which got implicitly fixed by the new code.

Also, somewhat related the "speeds up short path" you just commited actually most likely does not. :)

Check the generated code, it's surprisingly similar for both before and after.

http://goo.gl/XIPWyu

The inner loop is the same length, the only difference is the registers used, and the overall function size differs by 2 instructions, one of which is in the error handling that is rather unlikely to be triggered.

Again, the C compiler is good at obvious optimizations, so most of the time I go for the most easily readable verision of the code.

This change is otherwise fine, I guess, but the commit message is misleading.

So I guess having read_signed_int(int bytes) would be enough for now. Adding variants for specific lengths probably doesnt make a huge difference, as most of the runtime is spent in mega_apply and friends, especially when consuming just a couple of bytes.

I like the proposal for using negative lengths for little endian, so I would add that to read_int(int bytes) and read_signed_int(int bytes). I think we dont need explicit fixed length versions.

I personally prefer read_uint vs read_int as it avoids some possible confusion, but having read_signed_int would make it clear, too.

I am traveling right now, so not sure when I will be pushing a branch for those things...

arne

On 09/13/14 21:45, Stephen R. van den Berg wrote:

Per Hedbor wrote:

...

It would also break hstring reading rather badly, so you would still need the functions to read unsigned numbers internally.

...

Also, with native integers, as it turns out, it's fairly equal on x86_64 at least, where the integer operations are inlined.

...

Just add another function to read signed integers.

Shall we rename the read_int* to read_uint* then?

...

And as for byteorder, just add a switch to switch byteorder if the number of functions needs to be kept down for some reason.

Well, since IOBuffer is for efficiency and not for beauty, I guess it does not matter a lot if there are few or a lot of functions. Whatever is most efficient counts here, I'd say.

On Mon, Sep 8, 2014 at 10:21 PM, Stephen R. van den Berg srb@cuci.nl wrote:

The question now remains:

• Do we agree this is a bug?
• Or do we document that this is a feature?
• Or maybe we already documented it as a feature and I just didn't see it.

It's documented here:

http://pike.lysator.liu.se/generated/manual/modref/ex/lfun_3A_3A/_backtick_a...

But it's a distinct difference from, say, the Python equivalent, so it's worth making sure it's properly visible. Where would you go looking for that info?

ChrisA

On Mon, Sep 8, 2014 at 10:38 PM, Stephen R. van den Berg srb@cuci.nl wrote:

Well, for one, someone browsing the docs needs to be explained the difference between http://pike.lysator.liu.se/generated/manual/modref/ex/predef_3A_3A/_backtick... and http://pike.lysator.liu.se/generated/manual/modref/ex/lfun_3A_3A/_backtick_a...

Neither of these is for the += case, though. If you click the link to `+=, you get the info Tobias and I linked to.

ChrisA

AAAAAH! Nooooo... It's very deprecated.