Re: HEADS UP: Forth Optimizations
On Nov 11, 2012, at 5:30 PM, Peter Jeremy wrote:
On 2012-Nov-10 16:53:10 -0800, Devin Teske devin.te...@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?
Re: HEADS UP: Forth Optimizations
On 2012-Nov-10 16:53:10 -0800, Devin Teske devin.te...@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.
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.
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.
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
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]?
@@ -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.
@@ -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.
@@ -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.
@@ -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 )
@@ -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.
pgpjhm7HlFkWe.pgp
Description: PGP signature
HEADS UP: Forth Optimizations
Hi Everybody, Adrian (my co-mentor) wanted some additional eyes/names for review on a patch I'm making to sys/boot/forth (patch attached as patch.txt). The patch makes no changes to user experience or functionality (but _does_ fix one incident of stack leakage -- among other things). I wrote/tested this over a 2-day period using (as usual) VMware. I booted up several times (10+) and fiddled with many things (twiddled every knob, dropped to the loader prompt and went back to the menu several times, tried throwing various options like boot_single=YES and boot_verbose=YES into loader.conf(5) to make sure dynamic initialization is still working etc.). The only thing I noticed after applying the patch was a drop in heap usage (one of the goals of the patch), showing that the optimizations did their job to make a leaner running menu. Also, the code is a lot more readable and got slightly reduced in size during the process. Can someone help review this for the commit log? -- Devin === + This patch does not change user experience or functionality + Cleanup syntax, slim-down code, and make things more readable + Introduce new +c! operator and ilk to reduce heap usage/allocations + Fix a stack leak in [unused] cycle_menuitem function while we're here (required misconfiguration and/or missing environment vars to occur) + Add safemode_enabled? safemode_enable and safemode_disable functions + Add singleuser_enabled? singleuser_enable singleuser_disable functions + Add verbose_enabled? verbose_enable and verbose_disable functions + Centralize strings (also to reduce heap usage) PR: Submitted by: Reviewed by:Your_Name_Here, adrian (co-mentor) [pending his/your review] Approved by:adrian (co-mentor) [pending his approval] Obtained from: MFC after: Security: --This line, and those below, will be ignored-- Description of fields to fill in above: 76 columns --| PR:If a GNATS PR is affected by the change. Submitted by: If someone else sent in the change. Reviewed by: If someone else reviewed your modification. Approved by: If you needed approval for this commit. Obtained from: If the change is from a third party. MFC after: N [day[s]|week[s]|month[s]]. Request a reminder email. Security: Vulnerability reference (one per line) or description. Empty fields above will be automatically removed. Mforth/menu-commands.4th Mforth/menu.4th _ 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. Index: menu-commands.4th === --- menu-commands.4th (revision 242835) +++ menu-commands.4th (working copy) @@ -31,6 +31,10 @@ include /boot/menusets.4th variable kernel_state variable root_state +\ +\ ACPI +\ + : acpi_enable ( -- ) s set acpi_load=YES evaluate \ XXX deprecated but harmless s set hint.acpi.0.disabled=0 evaluate @@ -58,9 +62,38 @@ variable root_state TRUE \ loop menu again ; +\ +\ Safe Mode +\ + +: safemode_enabled? ( -- flag ) + s kern.smp.disabled getenv -1 dup if + swap drop ( c-addr flag -- flag ) + then +; + +: safemode_enable ( -- ) + s set kern.smp.disabled=1 evaluate + s set hw.ata.ata_dma=0 evaluate + s set hw.ata.atapi_dma=0 evaluate + s set hw.ata.wc=0 evaluate + s set hw.eisa_slots=0 evaluate + s set kern.eventtimer.periodic=1 evaluate + s set kern.geom.part.check_integrity=0 evaluate +; + +: safemode_disable ( -- ) + s kern.smp.disabled unsetenv + s hw.ata.ata_dma unsetenv + s hw.ata.atapi_dma unsetenv + s hw.ata.wc unsetenv + s hw.eisa_slots unsetenv + s kern.eventtimer.periodic unsetenv + s kern.geom.part.check_integrity unsetenv +; + : init_safemode ( N -- N ) - s kern.smp.disabled getenv -1 if - drop ( n c-addr -- n ) \ unused + safemode_enabled? if toggle_menuitem ( n -- n ) then ; @@ -70,25 +103,10 @@ variable root_state \ Now we're going to make the change effective - s toggle_stateN @ \ base name of toggle state var - -rot 2dup 12 + c! rot\ replace 'N' with ASCII numeral - - evaluate 0= if - s kern.smp.disabled unsetenv - s hw.ata.ata_dma unsetenv - s hw.ata.atapi_dma unsetenv - s hw.ata.wc unsetenv - s hw.eisa_slots unsetenv - s kern.eventtimer.periodic unsetenv - s kern.geom.part.check_integrity unsetenv + dup toggle_stateN
