[perl #16274] [PATCH] Keyed access
Dan Sugalski
>
>Attached is my first cut of a patch to address the keyed access issues.
>
>This patch doesn't do everything, but it does bring things more or less
>in line with Dan's recent specification I hope. I'm sure there are also
>problems with it so if we can get some eyeballs on it before I commit it
>that would be good.
>
>One thing it does not handle yet is dynamic key creation. I plan to address
>that next but I thought it would be a good idea to get this in first as it
>is already quite large and putting this in will enable other people to start
>working on things like making PerlHash handle multi level keys.
So this doesn't fall victim to Warnock's Dilemma... I'd appreciate it
if folks would go over this patch. If it makes sense, or next
Wednesday comes (whichever arrives first) lets commit it.
--
Dan
--------------------------------------"it's like this"-------------------
Dan Sugalski even samurai
d...@sidhe.org have teddy bears and even
teddy bears get drunk
Mike Lambert
First, thanks for spending the time to implement and clean up the keyed
code. Hopefully this'll clean the floor so that when this list has key
discussions, we'll all be arguing about the same thing. :)
> This patch doesn't do everything, but it does bring things more or less
> in line with Dan's recent specification I hope. I'm sure there are also
> problems with it so if we can get some eyeballs on it before I commit it
> that would be good.
Here are the things I noticed when going through your patch.
- assemble.pl:
shouldn't the code :
elsif ($_->[0] =~ /^([snpk])c$/) { # String/Num/PMC/Key constant
include support for "kic" somewhere?
the magic numbers in _key_constant, I'm assuming they are supposed to map
to the constants in key.h ? Perhaps a note mentioning that correspondance
would be useful. Also, it seems the number usage is broken. You use
1,1,1,2,4,7. Shouldn't it be 1,1,1,2,3,5? And shouldn't s/inps/ be
s/insp/? Or maybe the constants in key.h need rearranging?
- dod.c:
Near the comment, "Mark the key constants as live". Constants shouldn't
need to be marked live, as constants are prevented from being GC'ed, if
PMC_constant_FLAG is set. At least, in theory. Did it not work for you?
- core.ops
Looking at the set functions, shouldn't the "Px[ KEY ] = Bx"
set of functions have $1 defined as inout instead of out in most
circumstances?
In your definition of the groups of set functions, can you change "Ax =
Px[ KEY ]" to "Ax = Px[ INTKEY ]" where appropriate?
- key.pmc
the mark() function needs to return a value. Namely, the return value of
key_mark.
- random
Your use of registers for key atom values is interesting, and I think it
will create problems. It's not a problem with your patch as much a
problem with an aspect of the key design, I think.
The plan is to allow parrot functions to implement vtable methods in
parrot. If I have a key [I0,I1], and pass it to this vtable method, it
could be passed to a function implemented in parrot, with all of parrot's
calling conventions. This means that by the time it gets to the person
implementing the key, it's extremely possible that the registers have been
overwritten.
I'm not sure how to resolve this one. Alternatives are:
a) don't allow register references in keys. Instead, force people to use
the key modification ops to reset the key to the correct values each time
they want to use it.
b) handle auto-generated .ops files, such that if they receive a KEY as a
parameter, it calls key_fixup_registers, which grabs the current values of
the registers and sticks them into the key structure. This could cause
problems with constant keys, so you might need to create a key copy.
c) any other ideas? Or should we mark this as a 'known limitation' ?
Overall, tho, the patch looks extemely complete. Tracing support,
disassemble.pl support, debug.c support, etc. You even reduced macro
usage. Rather impressive. :)
Thanks,
Mike Lambert
Tom Hughes
Mike Lambert <pe...@jall.org> wrote:
> - assemble.pl:
> shouldn't the code :
> elsif ($_->[0] =~ /^([snpk])c$/) { # String/Num/PMC/Key constant
> include support for "kic" somewhere?
It doesn't need to as to_bytecode() turns [1] into an ic argument but
adds kic to the op name. Much the same thing is done for integer register
keys. So when _generate_bytecode() runs the argument type will just
appear to be a i or ic.
> the magic numbers in _key_constant, I'm assuming they are supposed to map
> to the constants in key.h ? Perhaps a note mentioning that correspondance
> would be useful. Also, it seems the number usage is broken. You use
> 1,1,1,2,4,7. Shouldn't it be 1,1,1,2,3,5? And shouldn't s/inps/ be
> s/insp/? Or maybe the constants in key.h need rearranging?
Actually they correspond to the PARROT_ARG_XX constants uses for encoding
op arguments types. I should really add a perl version of those constants.
> - dod.c:
> Near the comment, "Mark the key constants as live". Constants shouldn't
> need to be marked live, as constants are prevented from being GC'ed, if
> PMC_constant_FLAG is set. At least, in theory. Did it not work for you?
The reason I did it that way was that I wasn't sure whether a PMC that
was marked as constant could ever die before the end of the program, and
whether we might need to add and remove constant tables on the fly when
we load and unload bits of code.
Given that strings in the constant table are marked as constant I guess
that it should be safe to do the same for keys, so I have changed that.
> - core.ops
> Looking at the set functions, shouldn't the "Px[ KEY ] = Bx"
> set of functions have $1 defined as inout instead of out in most
> circumstances?
Possibly. That was copied from the original. I'm not quite sure
what difference inout makes to the code that is generated?
> In your definition of the groups of set functions, can you change "Ax =
> Px[ KEY ]" to "Ax = Px[ INTKEY ]" where appropriate?
Done.
> - key.pmc
> the mark() function needs to return a value. Namely, the return value of
> key_mark.
Oops. That only went in yesterday... Now fixed.
> Overall, tho, the patch looks extemely complete. Tracing support,
> disassemble.pl support, debug.c support, etc. You even reduced macro
> usage. Rather impressive. :)
The tracing, disassembly etc was mostly done when I found I needed
it to try and find a problem in the other things I'd done ;-)
One other outstanding problem that I remembered last night is that
it is allocating memory for the key atom which is attached to the
cache.struct_val member on the PMC but which is never freed.
Allocating that small piece of memory as a buffer which can be GCed
seems like complete overkill, but short of marking the PMC as having
a private GC so it can cleanup I hadn't managed to come up with a
solution.
What I realised last night however is that there is enough space in
the private flags on the PMC for the type information and I can then
attach the data directly to the cache and do away with key atoms
completely... I shall get on with that now and post a new patch later.
Tom
--
Tom Hughes (t...@compton.nu)
http://www.compton.nu/
Josef Hook
On Sun, 18 Aug 2002, Tom Hughes wrote:
> In message <837edc6...@compton.compton.nu>
> Tom Hughes <t...@compton.nu> wrote:
>
> > What I realised last night however is that there is enough space in
> > the private flags on the PMC for the type information and I can then
> > attach the data directly to the cache and do away with key atoms
> > completely... I shall get on with that now and post a new patch later.
>
> Here is the new patch - this includes the above change as well as
> most of the things Mike raised.
>
> It is also up to date with respect to the latest commits, including
> patching the MultiArray PMC to work with the new scheme. At least I've
> made the edits and think it will work, but as there don't seem to be
> any tests for it I can't be sure.
good job.
/Josef
Jeff
>
> In message <Pine.LNX.4.44.020818...@jall.org>
> Mike Lambert <pe...@jall.org> wrote:
> Oops. That only went in yesterday... Now fixed.
>
> > Overall, tho, the patch looks extemely complete. Tracing support,
> > disassemble.pl support, debug.c support, etc. You even reduced macro
> > usage. Rather impressive. :)
Agreed.
> The tracing, disassembly etc was mostly done when I found I needed
> it to try and find a problem in the other things I'd done ;-)
>
> One other outstanding problem that I remembered last night is that
> it is allocating memory for the key atom which is attached to the
> cache.struct_val member on the PMC but which is never freed.
>
> Allocating that small piece of memory as a buffer which can be GCed
> seems like complete overkill, but short of marking the PMC as having
> a private GC so it can cleanup I hadn't managed to come up with a
> solution.
>
> What I realised last night however is that there is enough space in
> the private flags on the PMC for the type information and I can then
> attach the data directly to the cache and do away with key atoms
> completely... I shall get on with that now and post a new patch later.
It's not quite applying against the current build, however.
classes/default.pmc was easy to fix, assemble.pl not so simple, core.ops
and hash.c had other problems. Could I trouble you to fix these so I can
commit it tonight? I can send you the rejected hunks if you like...
Jeff
Sorry about the QA comment, I just had someone point out to me that was
mail server mangling. Many apologies. It -really- looks like a nice
patch, if we can get these few problems ironed out.
--
Jeff <jg...@perl.org>
Tom Hughes
Jeff <drf...@hargray.com> wrote:
> Jeff wrote:
>
> > It's not quite applying against the current build, however.
> > classes/default.pmc was easy to fix, assemble.pl not so simple, core.ops
> > and hash.c had other problems. Could I trouble you to fix these so I can
> > commit it tonight? I can send you the rejected hunks if you like...
>
> Sorry about the QA comment, I just had someone point out to me that was
> mail server mangling. Many apologies. It -really- looks like a nice
> patch, if we can get these few problems ironed out.
I have a clean version that's up to date, and as everybody seems to
be happy with it I'm going to go ahead and commit it now.
Jeff
That I am. Thanks, Tom.
--
Jeff <jg...@perl.org>
Mike Lambert
> be happy with it I'm going to go ahead and commit it now.
Ah-ha! I found a showstopper! Oh, it's a little late for that, isn't it?
:)
Anyways, cd to languages/BASIC, run basic.pl, type "LOAD wumpus", and
watch it die on "Not a string!". It could be that basic is using keys in
weird ways, or it could be that the key patch is borked...I haven't looked
into it enough to determine the true cause here.
Thanks,
Mike Lambert
Tom Hughes
Mike Lambert <pe...@jall.org> wrote:
> Anyways, cd to languages/BASIC, run basic.pl, type "LOAD wumpus", and
> watch it die on "Not a string!". It could be that basic is using keys in
> weird ways, or it could be that the key patch is borked...I haven't looked
> into it enough to determine the true cause here.
The problem is that it is trying to index a hash using a number and
that is no longer allowed.
At some point it is going to be allowed again, but it won't do what
the code there wants it to do - it won't stringify the number and then
use that as a key.
What it will do is some sort of back door reference directly to an
entry in the hash table without going through a hash lookup. Dan has
said that this is needed for efficient access to scratchpads or
something although I'm not quite sure how it is supposed to work
given that the hash may decide to rearrange itself if it gets too
full.
What the BASIC interpreter probably needs to do is stringify those
keys itself before trying to look them up.
Mike Lambert
> Index: basicvar.pasm
> ===================================================================
...
> Index: instructions.pasm
> ===================================================================
...
Fixes the bug, and wumpus plays yet again.
Applied, thanks.
Mike Lambert
Leopold Toetsch
> Anyways, cd to languages/BASIC, run basic.pl, type "LOAD wumpus", and
> watch it die on "Not a string!".
Tracing this beast down, needs attached patch, to cut displayed arg strings.
BTW trace.c nether frees this escaped string.
The last instruction executed is:
PC=1457; OP=102 (set_p_ki_s); ARGS=(P22=0x81565d4, [I1=1], S0="1 REM
Taken from
David Ahl's 101 BASIC G...")
Not a string!
.... where actually the hash key I1 is not a string.
AFAIK this behaviour is intended.
The key patch is working fine, all perl6 tests succeed as before.
leo