Prevent word duplication when appending to an object

[0branch]

(Addresses Cure Code #1979.)

Ensure that duplicate words in block! arguments are flagged as temporary, favouring the final value.

The linear search isn't ideal, though it means that existing structures can be re-used without further allocation.

[BrianHawley]

So this is only happening in the case where the word isn't already in the object?

>> append context [b: 1] [b: 2 b: 3]
== make object! [
    b: 3
]

The make object! case uses code that is exported as the collect-words native. Might there be some better code in that native that you can use, like what it does to do collect-words/set? Perhaps not, it's just worth looking into.

[0branch]

Right: only when the word isn't already present.

Thanks for the suggestion, will take a look.

[0branch]

As indicated in the original message, the nested linear search is far from ideal and should be avoidable if we allocate a map! or similar structure.

Here is the performance degradation in the worst case (quadratic, O(n²)) where all words in the block are unique:

$ r3-1979 append-profile.r
Preparing input...words: 1000
Elapsed: 0:00:00.022868
~~
Preparing input...words: 10000
Elapsed: 0:00:01.915341
~~
Preparing input...words: 20000
Elapsed: 0:00:08.370466

vs.

$ r3-master append-profile.r
Preparing input...words: 1000
Elapsed: 0:00:00.000076
~~
Preparing input...words: 10000
Elapsed: 0:00:00.000636
~~
Preparing input...words: 20000
Elapsed: 0:00:00.000637

I don't find this acceptable; suggest we close this pull request (apologies for filing a naive patch) and treat as a demonstration of the duplication problem.

[BrianHawley]

Well, from a naive outside position (I haven't examined the code), let me suggest a possible solution expressed in Rebol code for append object block where block is presumed to possibly contain words :

foreach [w v] block [bind/new w object]
append object block

Basically, this is a block of pairs of any-word and value, no evaluation necessary, so we just need to add new words to the object; the bind/new function does that. So, use the method bind/new uses to add the words in the block itself (not deep into nested blocks) to the object first, before you do the append. That method is already hashed. Then, after that, do the existing append object block code, but now it will be the words-exist-in-object-already operation that we know already works, and that word lookup is hashed already too.

Then optimize a little, maybe doing things like just expanding the object once in the bind/new phase rather than n times. Does that make sense?

[0branch]

I think I follow. One issue though: the current code proceeds with append in two steps,

1. Verify the block structure (this is where duplicates are currently detected);
2. Append once verified.

The original patch aimed to preserve this logic.

If I'm reading your proposal correctly, you're mutating the block (by binding member words) iteratively. So: What happens when you arrive at a bad key?

[BrianHawley]

"If I'm reading your proposal correctly, you're mutating the block (by binding member words) iteratively. So: What happens when you arrive at a bad key?"

The bind/new function when binding a word rather than a block doesn't mutate the word, it just generates a new word value with references to the old symbol and the new context. You were missing the important bit, which is the /new part, which adds any new words found to the object (with unset values at first) - that is the only mutating part. It was just a hint of where to go to find code that collects a set of words and adds them to an object.

"The alternative solution I had in mind was..."

You don't need to allocate a new object, you already have an object that you are going to be adding the values to anyway, so you might as well use it. It would mean 3 fast passes, the native equivalent of this Rebol code:

forskip block 2 [unless any-word? first block [trap]]  ; Use whatever Trap function it uses in native
forskip block 2 [bind/new first block object]  ; This modifies object, copies the word
forskip block 2 [set/any in object first block second block]  ; value will be none if block ends early

Now you might want to optimize the second step by looking at what the /new option of bind does when it has a block of words that it is adding to the object all at once, and maybe do something like that. Or you can do it one-at-a-time. You don't actually need to do any of the actual binding in the second step, unless what you are doing is generating a new block with bound words and unaltered value references, which might be faster because it would let you skip the in object part in the third pass - test to be sure.

You might also consider looking at what reduce and compose do in this situation. Instead of allocating a new series, they just push their temporary values on the stack, then copy them into the results and just let the function return get rid of the temporaries (the stack is basically a block, FYI). If you think that the block appended is going to be relatively small (100 items or less maybe) you can push your bound words to the stack in step 2, then just set them in step 3. It is trading heap pressure until the next GC, for stack pressure until the function returns, but that is likely a good tradeoff when you consider that it doesn't recursively call other functions.

[0branch]

Thanks for the clarification.

I tested your bind/new proposal this morning (the naive version where binding is performed a word at a time).

Here're some rough worst-case results (all words are unique, the target object! is empty):

• Using bind/new (frame appending when a word isn't bound in the target object),

  Preparing input of 100 unique words ... done.
  Append duration: 0:00:00.000049
  ~~
  Preparing input of 1000 unique words ... done.
  Append duration: 0:00:00.00089
  ~~
  Preparing input of 10000 unique words ... done.
  Append duration: 0:00:00.085974
  ~~
  Preparing input of 100000 unique words ... done.
  Append duration: 0:00:08.816506
  ~~
  Preparing input of 200000 unique words ... done.
  Append duration: 0:00:41.362606
  

• Current HEAD of master,

  Preparing input of 100 unique words ... done.
  Append duration: 0:00:00.000023
  ~~
  Preparing input of 1000 unique words ... done.
  Append duration: 0:00:00.000034
  ~~
  Preparing input of 10000 unique words ... done.
  Append duration: 0:00:00.001611
  ~~
  Preparing input of 100000 unique words ... done.
  Append duration: 0:00:00.010069
  ~~
  Preparing input of 200000 unique words ... done.
  Append duration: 0:00:00.014229
  

Another suggestion: how about leveraging resolve for this purpose? In particular, resolve/extend/all? Consider,

>> append context [a: 1] [a: 2 b: 3 b: 4]
== make object! [
    a: 2
    b: 4
]
>> resolve/extend/all context [a: 1] context [a: 2 b: 3 b: 4]
== make object! [
    a: 2
    b: 4
]

By converting the block! at the outset, we end up with a blazing fast "append",

Preparing input of 100 unique words ... done.
Employing resolve...
Duration: 0:00:00.000047
~~
Preparing input of 1000 unique words ... done.
Employing resolve...
Duration: 0:00:00.000222
~~
Preparing input of 10000 unique words ... done.
Employing resolve...
Duration: 0:00:00.003199
~~
Preparing input of 100000 unique words ... done.
Employing resolve...
Duration: 0:00:00.019207
~~
Preparing input of 200000 unique words ... done.
Employing resolve...
Duration: 0:00:00.047758
~~
Preparing input of 1000000 unique words ... done.
Employing resolve...
Duration: 0:00:00.225966
~~
Preparing input of 5000000 unique words ... done.
Employing resolve...
Duration: 0:00:01.232638

[0branch]

Unfortunately, the above implementation introduces inconsistent state when attempting to set protected words during an append,

>> protect in x: context [a: 1] 'a
== a
>> append x [b: 2 c: 3 a: 4]
** Script error: protected variable - cannot modify: a:
** Where: append
** Near: append x [b: 2 c: 3 a: 4]
>> x
== make object! [
    a: 1
    b: 2
    c: 3
]    

Note that while master fares slightly better, backout is still problematic:

>> protect in x: context [a: 1] 'a
== a
>> append x [b: 2 c: 3 a: 4]
** Script error: protected variable - cannot modify: a:
** Where: append
** Near: append x [b: 2 c: 3 a: 4]
>> x
== make object! [
    a: 1
]
>> append x [b: 2]
== make object! [
    a: 1
    b: 2
]
>> append x [b: "whoops" c: 3 a: 4]
** Script error: protected variable - cannot modify: a:
** Where: append
** Near: append x [b: "whoops" c: 3 a: 4]
>> x
== make object! [
    a: 1
    b: "whoops"
]

Since new frames are appended once mutation of existing words has occurred, the problem only becomes apparent under certain ordering conditions (b is set before the trap on a).

[earl]

Well, you'd "just" have to move the protected check back to the verification loop to get rid of the inconsistent state problem. (Which would be another fix over r3-master, as you already noted.)

Unfortunately, Find_Word_Index is just another linear search in disguise (over the target object's frame) ... So we have O(n_m) in the protected checking, and another O(n_m) in the final mutation (in the worst case, of course). That is a slight improvement over the initial O(n²), as we'd now need both huge target objects and huge to-be-appended blocks to make this bite; but it still strikes me as problematic. (Note, however, that the original code is also O(n*m).)

[0branch]

Agreed on both points. I'll push a verification fix shortly for completeness, though I don't find these solutions particularly satisfactory.

Incidentally, since I suggested it above, note that resolve/extend/all doesn't perform any verification:

>> protect in x: context [a: 1] 'a
== a
>> resolve/extend/all x context [a: "ignored" b: 2]
== make object! [
    a: 1
    b: 2
]
>> resolve/extend/all x context [b: "replaced" c: 3]
== make object! [
    a: 1
    b: "replaced"
    c: 3
]

—protected fields are simply left unchanged.

[0branch]

Some profiling data for the latest commit (46e3d14),

Here the target object consists of 500000 words, disjoint from the input.

Preparing input of 100 unique words ... done.
Append duration: 0:00:00.225658
~~
Preparing input of 1000 unique words ... done.
Append duration: 0:00:02.371522
~~
Preparing input of 10000 unique words ... done.
Append duration: 0:00:22.006266
~~
Preparing input of 100000 unique words ... done.
Append duration: 0:03:49.309265

As indicated in the output, the target object is created with a fixed size (500,000 words) and is completely disjoint from the source block. This means that the entirety of the target needs to be walked per candidate arg during verification.

[ladislav]

#153 is meant to replace this

[carls]

Pull request #153 accepted - closing this one.