From: SourceForge.net <no...@so...> - 2004-09-22 15:48:44
|
Bugs item #729692, was opened at 2003-04-29 13:27 Message generated for change (Comment added) made by msofer You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=729692&group_id=10894 Category: 46. Bytecode Compiler Group: development: 8.5a2 Status: Closed Resolution: Fixed Priority: 7 Submitted By: Reinhard Max (rmax) Assigned to: miguel sofer (msofer) Summary: redef of bcc'ed commands inoperant in current bcode Initial Comment: The following piece of code doesn't work as expected due to the empty proc body optimization that went into Tcl 8.4. proc bar args {} proc main {} { proc bar args {puts "this is bar"} bar } main While this example is artificial, I stumbled over this bug in a situation where a (formerly empty) proc got redefined through by namespace import -force and invoked immediately afterwards, all within the same arm of a switch statement. So it took me some time (and the help of fellow Tcl'ers on the chat) to find out what's going on. ---------------------------------------------------------------------- >Comment By: miguel sofer (msofer) Date: 2004-09-22 12:48 Message: Logged In: YES user_id=148712 The fix has been corrected; it was wrong whenever the newly-interpreted script returns TCL_BREAK or TCL_CONTINUE. ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2004-03-30 15:10 Message: Logged In: YES user_id=148712 Closed again - performance bug 926164 now open. ---------------------------------------------------------------------- Comment By: Jeffrey Hobbs (hobbs) Date: 2004-03-30 14:20 Message: Logged In: YES user_id=72656 IMO this patch should be reversed if miguel's slowdown numbers are accurate. This was a known issue, and is a pathological case that is acceptable when documented. It's not acceptable to take up to a 10% slowdown for the core just to support this. ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2004-03-30 13:38 Message: Logged In: YES user_id=148712 Patch committed; we'll improve on it during alpha if necessary. New bug item for the missing async checks during empty loops. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2004-03-30 12:49 Message: Logged In: YES user_id=80530 I'm all in favor of making further improvements, but how about committing the fix we have to the HEAD? ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2004-03-30 09:57 Message: Logged In: YES user_id=79902 We need to work out what duties the ISC serves and optimise it for that. The more it does, the less it can be optimised. ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2004-03-30 08:24 Message: Logged In: YES user_id=148712 (Topic now changed to discussion of the patch ... OT for the bug ticket? Should I rather open a patch ticket?) Further possible usage for ISC (assuming no redundancy elimination): enabling of command tracing in bcc'ed code. ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2004-03-30 08:20 Message: Logged In: YES user_id=148712 The minimisation of INST_START_CMDs (ISC) looks to me like (yet another) job for a post-compile optimiser. In general, if there are two ISCs such that: (a) there is no intervening jump or jump target (including the second ISC) (b) there is no intervening invocation or variable access (in TEBC terms: instructions that do not DECACHE_STACK_INFO) then the second ISC can be effectively eliminated. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2004-03-30 06:43 Message: Logged In: YES user_id=79902 Hmm. If the target was the next instruction, that'd be OK. However, we'd also need to be careful to only do this in the case when the body was empty (or at least empty of bytecodes). In the non-empty case, the body's own startcmds would be better. Without this particular niggle, I think it would be fairly easy to add the extra information to the compileenv to help generate only minimal numbers of startcmds, which would probably help with the speed. ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2004-03-30 06:24 Message: Logged In: YES user_id=148712 I was actually thinking of compiling an INST_START_CMD into every empty loop body. Any downside to that? ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2004-03-30 05:08 Message: Logged In: YES user_id=79902 An idea that occurred to me is that we should not issue this instruction multiple times in a row (currently possible when compiling a command whose argument is itself a compiled command) since the first instance will more than adequately protect all subsequent ones. On the other issue: Perhaps we need a counter to do the asynch stuff that is reset when a startcmd instruction is processed? We could make the counter have a relatively long period, of course, so only very evil BC loops would have any danger of hitting it. ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2004-03-29 21:56 Message: Logged In: YES user_id=148712 Note that this patch introduces a new bug that requires fixingbefore applying: while 1 {} does not check for asyncs. ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2004-03-29 21:41 Message: Logged In: YES user_id=148712 Slightly modified patch and tclbench results attached. The patch produces a small overall slowdown in the whole benchmark suite. Bytecode intensive tests are noticeably slowed down: 5-10% (loops, base64, klist, list iterate, ...) with two bad ~15-20% cases (sha1, heapsort). ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2004-03-29 17:45 Message: Logged In: YES user_id=79902 It all looks like a good patch to me taking a good route to solve the problem, but I can't test ATM because of the other things I've got cooking in various sandboxes; I assume that the updated test suite works? ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2004-03-29 15:10 Message: Logged In: YES user_id=148712 Fixed patch to work correctly with precompiled scripts - the new instruction is effectively a noop for them. ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2004-03-29 00:22 Message: Logged In: YES user_id=148712 Enclosed a tentative patch for review; more test cases needed. Note that this also fixes [Bug 495830]. ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2004-03-10 11:02 Message: Logged In: YES user_id=148712 Idea - but may be costly in runtime. Define INST_START_CMD and INST_END_CMD. Let the bc engine have two states: - normal N: execute bytecodes - exceptional E: find next command from the source, interpret it State N: INST_START_CMD: check epoch; if ok do nothing, else switch state to E and rexec INST_START_CMD INST_END_CMD: a noop State E: INST_START_CMD: find in the sources the command corresponding to the next command; interpret it and push its result on the stack; jump to the instruction after the corresponding INST_END_CMD INST_END_CMD: a noop It can possibly be made marginally more efficient (in the sense of it being less costly in state E). But I do think this approach could work. Feedback? ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2004-03-10 08:54 Message: Logged In: YES user_id=79902 I think we're not talking 'impossible' but rather 'exceptionally difficult' and possibly even 'impossible with the current bytecode engine'. The nub of the problem is what happens if you redefine [while] inside a [while] loop and then try to call that redefined [while] while still executing that loop. You have to be able to invalidate sections of bytecode and replace them with bytecodes of potentially different length without invalidating the outer bytecodes which are still in use (change those and you're also altering the semantics of Tcl, and it's a non-trivial change at best.) ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2004-03-09 21:21 Message: Logged In: YES user_id=148712 This is very similar to bug 495830, at least in its causes. The general solution could well involve defining a new bytecode for either 'command start' or 'command end' that checks for (interp validity, epochs, ...) and switches to direct string evaluation in exceptional cases. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2004-03-09 19:16 Message: Logged In: YES user_id=80530 OK, this may go nowhere, but here's a thought. We have epoch counters that tell us whether we need to recomile a whole unit of bytecode before executing it. Some bytecode execution can increment the epoch counter. Can't we check for an increment after we execute those bytecodes, and if we see one, abandon bytecode execution and switch over to direct string evaluation? (leaving any recompile for the next evaluation of the whole bytecode unit) Is this merely difficult, or fundamentally impossible? ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2003-11-15 20:24 Message: Logged In: YES user_id=148712 Not much that we can do about that? This is really a fundamental bug in the bytecode model: once a bytecode is running, it can't be modified. In your example, changes to bytecompiled commands called from within main's body will not affect main while it is running. Another example is proc tst {} { rename set _set proc set args {puts GOTCHA} set x 1 rename set {} rename _set set } Running tst the new definition of 'set' will be ignored,as it was bytecompiled. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=729692&group_id=10894 |