HEADS UP: Forth Optimizations

Devin Teske devin.teske at fisglobal.com
Mon Nov 12 18:47:44 UTC 2012


On Nov 11, 2012, at 5:30 PM, Peter Jeremy wrote:

> On 2012-Nov-10 16:53:10 -0800, Devin Teske <devin.teske at fisglobal.com> wrote:
>> Can someone help review this for the commit log?
> 
> I've had a look through the proposed patch and my comments follow.
> Other than that, it looks good to me.
> 

Thanks Peter!

Replies inline below.


>> Index: menu-commands.4th
>> ===================================================================
>> --- menu-commands.4th	(revision 242835)
>> +++ menu-commands.4th	(working copy)
> ...
>> @@ -185,21 +240,21 @@ variable root_state
> ...
>> 	s" set kernel=${kernel_prefix}${kernel[N]}${kernel_suffix}"
>> -	                          \ command to assemble full kernel-path
>> -	-rot tuck 36 + c! swap    \ replace 'N' with array index value
>> -	evaluate                  \ sets $kernel to full kernel-path
>> +	36 +c! \ replace 'N' with ASCII numeral
>> +	evaluate
> 
> I think the "sets $kernel to full kernel-path" comment is worth keeping.
> 

Updated, thx.


>> 	s" set root=${root_prefix}${root[N]}${root_suffix}"
>> -	                          \ command to assemble root image-path
>> -	-rot tuck 30 + c! swap    \ replace 'N' with array index value
>> -	evaluate                  \ sets $kernel to full kernel-path
>> +	30 +c! \ replace 'N' with ASCII numeral
>> +	evaluate
> 
> Likewise, this could do with a (corrected) comment that it sets $root
> to the full path to root.
> 

Likewise, updated.


>> Index: menu.4th
>> ===================================================================
>> --- menu.4th	(revision 242835)
>> +++ menu.4th	(working copy)
>> @@ -184,18 +223,15 @@ create init_text8 255 allot
>> 
>> 		\ base name of environment variable
>> 		loader_color? if
>> -			s" ansi_caption[x]"
>> +			dup ansi_caption[x]
>> 		else
>> -			s" menu_caption[x]"
>> +			dup menu_caption[x]
>> 		then	
> 
> Could this be simplified to
> 
> = 		dup
> = 		loader_color? if
> =			ansi_caption[x]
> = 		else
> =			menu_caption[x]
> = 		then	
> 

Sure, done. Actually, found two occurrences of this that I corrected.


> Or, at a higher level, should this whole block be pulled into a new
> word (along with similar words for toggled_{ansi,text}[x] and
> {ansi,menu}_caption[x][y]?
> 

I looked at the uses where ansi* is used in place of non-ansi* and it's not just within loader_color? blocks (that was in-fact the minority of cases). Cooking it down further would make things more complicated I fear.

If I come up with a cute way to simplify this, I'll try it out in another commit.



>> @@ -227,36 +263,26 @@ create init_text8 255 allot
> ...
>> 		getenv dup -1 <> if
>> 			\ Assign toggled text to menu caption
> 
> Some comments on stack contents around here would make it somewhat
> easier to follow what is going on.
> 

Done. I also made updates to cycle_menuitem for similarly-dense code.


>> @@ -329,19 +340,18 @@ create init_text8 255 allot
> ...
>> 			\ This is highly unlikely to occur, but to make
>> 			\ sure that things move along smoothly, allocate
>> 			\ a temporary NULL string
>> 
>> +			drop ( getenv cruft )
>> 			s" "
>> 		then
>> 	then
> 
> Is this the memory leak?  If so, can I suggest that this be commited
> separately since it is a simple change and is distinct from the other
> changes you are proposing.

You got it. Committed as r242923


> 
>> @@ -357,14 +367,14 @@ create init_text8 255 allot
>> 	\ 
>> 	\ Let's perform what we need to with the above.
>> 
>> -	\ base name of menuitem caption var
>> +	\ Assign array value text to menu caption
>> +	4 pick
> 
> According to the docementation just above this hunk, there are only 4
> items on the stack, so "4 pick" seems wrong, though it is consistent
> with my understanding of the old code.  The "2 pick [char] 0" you
> added earlier seems to similarly be out-by-one, though consistent.
> 

My mistake was that the comments need to be updated to say C-Addr/U not C-Addr (the C-Addr/U counts to make 5 items on the stack, satisfying the "4 pick"). I've updated the comment to reflect the stack contents more accurately.


>> @@ -521,17 +528,20 @@ create init_text8 255 allot
>> 
>> 		\ If this is the ACPI menu option, act accordingly.
>> 		dup menuacpi @ = if
>> -			acpimenuitem ( -- C-Addr/U | -1 )
>> +			dup acpimenuitem ( n -- n n c-addr/u | n n -1 )
>> +			dup -1 <> if
>> +				13 +c! ( n n c-addr/u -- n ) \ replace 'x'
> 
> I think the stack here should be ( n n c-addr/u -- n c-addr/u )
> 

Good catch! Updated.



>> @@ -950,100 +914,43 @@ create init_text8 255 allot
>> 
>> 	49 \ Iterator start (loop range 49 to 56; ASCII '1' to '8')
>> 	begin
>> -		\ Unset variables in-order of appearance in menu.4th(8)
> 
> Does the order matter?  I notice you've changed it.

No, order doesn't matter. It was just a comment to myself.
Since I've re-ordered things, the axiom is no longer true.

Please find an updated patch attached (as patch.txt) with the aforementioned changes.
-- 
Devin

_____________
The information contained in this message is proprietary and/or confidential. If you are not the intended recipient, please: (i) delete the message and all copies; (ii) do not disclose, distribute or use the message in any manner; and (iii) notify the sender immediately. In addition, please be aware that any message addressed to our domain is subject to archiving and review by persons other than the intended recipient. Thank you.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch.txt
URL: <http://lists.freebsd.org/pipermail/freebsd-current/attachments/20121112/b9949188/attachment.txt>


More information about the freebsd-current mailing list