Menu
▾
▴
[Tcl-bugs] [ tcl-Patches-644819 ] Bytecompiled [switch] Command
|
From: <no...@so...> - 2002-12-12 16:05:46
|
Patches item #644819, was opened at 2002-11-27 16:36 You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=310894&aid=644819&group_id=10894 Category: 44. Bytecode Compiler Group: None Status: Open Resolution: None Priority: 5 Submitted By: Donal K. Fellows (dkf) Assigned to: Jeffrey Hobbs (hobbs) Summary: Bytecompiled [switch] Command Initial Comment: Here's a first attempt at byte-compiling the [switch] command; it needs a lot more work doing, but this should be enough for people to see if I'm going about it the right way. ---------------------------------------------------------------------- >Comment By: Donal K. Fellows (dkf) Date: 2002-12-12 16:05 Message: Logged In: YES user_id=79902 Fine with me. But you knew that already. :^) ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2002-12-12 16:02 Message: Logged In: YES user_id=80530 Given the inexperience of the author and the reviewers, I'd be more comfortable seeing this patch applied to an 8.5a HEAD, instead of the current 8.4.1.1 HEAD. Time to branch? ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2002-12-12 15:16 Message: Logged In: YES user_id=79902 Writing a piece of code to bytecode compile a command, and especially one that has sub-scripts like [if], [for], [while] and [switch] is really much harder than it looks. There's a lot of undocumented assumptions out there. (I think that it should be easier to generate bytecode for commands that don't have such things. Almost straight forward once you understand what's going on with jump fixups, stack depths and the bytecode models themselves.) About the specific points you raise: I think we should have split the 8.4.1 branch off immediately after the 8.4.0 release. :^) The comment refers to the fact that we don't care if the switch was called switch, or "switch" or {switch} or whatever; if we get to this code, we assume we know that we're at the right spot. And that comment only refers to the next line. The ones after that check that we've got options that we understand; not all of them are like that, and there's not a lot of point in trying to compile switches with -regexp as there's no neat way to generate bytecode for them right now (I'd need to add bytecodes too, an INST_RE_MATCH if nothing else.) I just handle the cases where we might get a real benefit. The other comment that you pick out relates to the fact that we are in the middle of handling a {key body key body ...} construct, and that is really difficult to do because you can't operate on a copy of the source (a fact which cost me about a day's solid work to discover and fix!) ---------------------------------------------------------------------- Comment By: Vince Darley (vincentdarley) Date: 2002-12-12 14:44 Message: Logged In: YES user_id=32170 Shouldn't we branch out a 8.4.1 branch of tcl/tk before applying any somewhat experimental patches like this to the HEAD? My brief comments (since I was interested in how this whole byte-compiling thing works!)... + /* + * We don't care how the command's word was generated; we're + * compiling it anyway! + */ + tokenPtr += tokenPtr->numComponents + 1; this comment doesn't help me too much. What's going on in the next 20 lines of code? + /* + * Test to see if we have guessed the end of the word + * correctly; if not, we can't feed the real string to the + * sub-compilation engine, and we're then stuck and so have to + * punt out to doing everything at runtime. + */ this suggests that for some values of 'word' we're not going to compile the switch, but that seems to contradict the earlier comment which only says we'll compile '--', '-glob --', '-exact --'. My conclusion: you are right, it is very complicated!! ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2002-12-12 09:44 Message: Logged In: YES user_id=79902 Miguel's told me that he's too busy to review this right now; should I apply it as is or do you (or any other Tcl maintainer) want to review it first? ;^) ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2002-12-11 15:39 Message: Logged In: YES user_id=79902 Writing a piece of code to compile a command is really much harder than it looks. >:^( Here's a version that seems to get *everything* right, insofar as it has no strange crashes in the test suite any more. ;^) Only thing I can't tell offhand is whether it has any memory leaks. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2002-12-11 00:15 Message: Logged In: YES user_id=79902 OK, the code was wrong. It fixed up some jumps far too early. Here's a version that puts off all fixing of jumps until after it has laid out all the code. And is actually simpler too. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2002-11-29 09:07 Message: Logged In: YES user_id=79902 Hmm. Something is still not right with the patch, given how many tests fail when it is applied. I think this needs going over by someone with more experience with the compiler side of things... ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2002-11-28 15:05 Message: Logged In: YES user_id=79902 Ooops! Comment in code was very misleading... ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2002-11-28 14:57 Message: Logged In: YES user_id=79902 Apart from the points Jeff raises (and which should be addressed by the new patch below), there were a great number of problems (relocation fixups and max stack depths were both broken!) (BTW, INST_STR_MATCH wasn't a straight slot-in replacement for INST_STR_EQ because it matters which argument is which. INST_OVER is our savour!) Performance wise on a 700MHz P3 Linux box: % proc foo s { set x 0; set y 0 foreach c [split $s {}] { switch -exact -- $c { a {incr x} b {incr y} } } return $x,$y } % proc bar s { set x 0; set y 0 foreach c [split $s {}] { switch -exact -- $c a {incr x} b {incr y} } return $x,$y } % time {foo shdgsdbcbcsjdsbvbsdjfvbsdbvsldfgsdfsvbsdhfbvlsdvsdbvldvxnvascv} 1000 275 microseconds per iteration % time {bar shdgsdbcbcsjdsbvbsdjfvbsdbvsldfgsdfsvbsdhfbvlsdvsdbvldvxnvascv} 1000 328 microseconds per iteration ---------------------------------------------------------------------- Comment By: Jeffrey Hobbs (hobbs) Date: 2002-11-27 23:26 Message: Logged In: YES user_id=72656 BTW, handling -glob would be easy - check for the -glob (and optionally --), and call TclEmitInst1(INST_STR_MATCH, 0 /* nocase */, envPtr); optionally instead of TclEmitOpcode (INST_STR_EQ, envPtr); You can look at TclCompileRegexpCmd to reuse some of that code. ---------------------------------------------------------------------- Comment By: Jeffrey Hobbs (hobbs) Date: 2002-11-27 23:17 Message: Logged In: YES user_id=72656 also the default case in the compiled command isn't checking to make sure that it is the last case, whereas the runtime one does. ---------------------------------------------------------------------- Comment By: Jeffrey Hobbs (hobbs) Date: 2002-11-27 23:14 Message: Logged In: YES user_id=72656 one thing that would need changing is that compiled commands should never throw an error - they should always throw TCL_OUT_LINE_COMPILE to let the error be raised at runtime (in the odd but possible situation that switch is overridden). ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=310894&aid=644819&group_id=10894 |