319719 - Atomless keywords

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently the scanner to recognize JS keywords lookups the atom table which is prefilled with keyword definitions. It requires to acquire a lock for each keyword and pollutes the global atom table with words that almost never used as names.

It would be nice if this can be avoided via some precomputed hashtable of keywords that can be looked up without any locks.

Comment 1

[Igor Bukanov]

Attachment: Proof of concept implementation

The patch replaces lookup of keywords in the global atom table by precomputed at compile time keyword selector code.

The basic idea is to move keywords definitions into jskeyword.tbl that is used by a new file, jskwswgen.c to generate a keyword switch code and place it to  jskwsw.gen. The jsscan.c includes the file and jskeyword.tbl to build all the necessary structures for keyword lookup and enumeration at compilation time.

The code tries to minimize amount of separated string constants defined and borrow few tricks from Appendix B of http://people.redhat.com/drepper/dsohowto.pdf to achieve that in jsscan.c

jskwswgen.c itself is adaptation of Rhino tool to generate string switch code.

The patch works but still requires some work especially in the Makefile area - currently only Makefile.ref is patched to define dependencies and rules for new files.

Still I would like to get some input on it and hence the review request.

Comment 2

[Igor Bukanov]

Attachment: Patch that compiles

The previous patch was incomplete and here is the correct version.

Comment 3

[Brendan Eich [:brendan]]

Interesting patch.  How much bigger is the code due to this?  How much, if any, data reduction does this win?

And mostly, since all identifiers that aren't keywords have to be atomized anyway, and given the JSThinLock used around the atom table, how much time for typical scripts, if any, does this save?

I don't believe hash loading of the atom table due to keywords actually costs non-keyword probes any significant cost.  With the JSThinLock, it's not clear this patch actually wins, so "it would be nice" ;-) to get head-to-head measurements of SpiderMonkey performance (JS_THREADSAFE, of course) with and without this patch.  Bob might be able to help test browser perf.  What benchmarks should we use?

Good to see performance thinking going on.  Just remember, measure twice, cut once :-).

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Interesting patch.  How much bigger is the code due to this?

The patch increases the size of optimized jsscan.o (Linux i386 with GCC 4.0.1) by about 2K from from 16928 to 18892. 

>  How much, if any,
> data reduction does this win?

The patch removes the need to atomize 59 keywords with 390 characters (including 59 trailing 0).

> 
> And mostly, since all identifiers that aren't keywords have to be atomized
> anyway, and given the JSThinLock used around the atom table, how much time for
> typical scripts, if any, does this save?
> 
> I don't believe hash loading of the atom table due to keywords actually costs
> non-keyword probes any significant cost.  With the JSThinLock, it's not clear
> this patch actually wins, so "it would be nice" ;-) to get head-to-head
> measurements of SpiderMonkey performance (JS_THREADSAFE, of course) with and
> without this patch.  Bob might be able to help test browser perf.  What
> benchmarks should we use?

I will need to extend the patch to include the new makerules for the generated code into Makefile.in before that. But eeven for jsshell the patch speeds up by about 5% benchmark like:


var line = 'if (x) ++i;\n'
Script(Array(30000).join(line));

while slowing down by 0.1% the slightly altered case of:

var line = 'if (xxx) ++iii;\n'
Script(Array(30000).join(line));

which shows that patch does not harm single-threaded case.

> 
> Just remember, measure twice, cut once :-).
> 
Surely, but the patch is a preliminary work to try another optimization later. 

The idea is to reduce string allocation/atomization cost during parsing via storing all string characters in a buffer so parsed tree nodes would contain indexes into the buffer instead of JSString pointers. When the parsing is done, the chars would be allocated into one "gigantic" JSString and strings for the emitter would be constructed as dependent substrings of this gigantic string.

This would reduce number of synchronization locks and malloc calls just to very few and may even bring clearly wisible benefits even for single-threaded case. But all this require unbundling keyword lookup from the global atom table and hence this patch.

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> But all this require unbundling keyword lookup from the global atom table and
> hence this patch.
> 

And if testing would show that the patch brings benefits alone, then even better.

Comment 6

[Brendan Eich [:brendan]]

D'oh -- I wasn't cc'd.

Igor, this plan sounds great.  I saw your comment in the FastLoad bug, and it'll help there too.  The code size increase is trivial, but I want to measure the total keyword density (ratio of reserved to unreserved identifiers) in Firefox, and perhaps a few other large-scale apps.  I suspect it's lower than in the small synthetic benchmarks in comment 4.

Whether a lower keyword density (Dk) matters depends on costs:

Catom   cost of atomizing an identifier
Ckhit0  cost of a keyword hit in current (0-based) code
Ckhit1  cost of hit on average given keyword frequency with this bug's patch
Ckmiss1 cost of a miss on average, etc., with this bug's patch

Trade-off relation: cost before patch <?> cost after patch

Catom + Dk*Ckhit0 <?> Dk*Ckhit1 + (1 - Dk)*(Ckmiss1 + Catom)
Catom + Dk*Ckhit0 <?> Dk*Ckhit1 + Ckmiss1 - Dk*Ckmiss1 + Catom - Dk*Catom
        Dk*Ckhit0 <?> Dk*Ckhit1 + Ckmiss1 - Dk*Ckmiss1 - Dk*Catom
           Ckhit0 <?> Ckhit1 + Ckmiss1/Dk - Ckmiss1 - Catom

If Catom >> all other costs and Dk is not too small, then clearly the relation <?> is > and the patch wins.

If Dk is small enough of a fraction, and Ckmiss1 multiplied by Dk's reciprocal is big enough, then perhaps the patch does not win.  Let's find out, because this was a win for Rhino, and the JSThinLock involved in much of Catom is similar to Java's bacon-bit optimized synchronized method/block locks.

/be

Comment 7

[Brendan Eich [:brendan]]

           Ckhit0 <?> Ckhit1 + Ckmiss1/Dk - Ckmiss1 - Catom

rearranging:

   Catom + Ckhit0 <?> Ckhit1 + Ckmiss1*(1 - Dk)/Dk

This shows better how Catom (as Igor pointed out, irrespective of JS_THREADSAFE) almost certainly dominates.  Even if Dk were .2, we'd have Catom + epsilon <?> small + 4 * small, and if Catom is large, then <?> must be >.

Dk from patch to follow in next attachment, applied to my not-up-to-date trunk Firefox, with default home page for developer builds, after running on a profile where I rm'ed history.dat and XUL.mfasl, and shut down: 0.283401.

JS is keyword-happy!  When I showed the Y combinator in JS to the ACM ICFP conference attendees, this was all too clear.  Oh well, it's good for beginners, right?  Hmm.

/be

Comment 8

[Brendan Eich [:brendan]]

I wrote:

> the total keyword density (ratio of reserved to unreserved identifiers) in

but of course, it's (ratio of reserved to all identifiers).

Reviewing patch in a moment.  This should be targeted at 1.9alpha.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: patch to measure cumulative Dk and report it on every GC

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 205414 [details] [diff] [review]
Patch that compiles

I'd use uint16 for offset and length, with an assertion that the total size of keyword_chars_data is < JS_BIT(16).

Eliminate js_InitScanner, it's not exported so we can do without it until such time as it is needed again.  CVS will remember ;-).

FindKeyword needs to cast keyword[i] to (uint8) to avoid a warning on targets that have signed chars, I think.

Mega-nit: js_IsIdentifier might rather declare jschar *chars, *end, *s, c; or jschar c, *chars, *end, *s; to balance order-of-first-def and "scalars before pointers or vice versa" desiderata.

Nit: ATOM_IS_IDENTIFIER macro body is unnecessarily parenthesized.

Nit in jskwswgen.c:

            ++nchar; current_char = c;

should be two lines, at least to make count_different_chars match count_different_lengths.

r=me, but we need Makefile.in updated, and we need this test-built on all platforms before checkin.

/be

Comment 11

[Igor Bukanov]

(In reply to comment #7)
>    Catom + Ckhit0 <?> Ckhit1 + Ckmiss1*(1 - Dk)/Dk

Given the structure of the switch from jskwsw.gen produces by the patch, Ckhit1 and Ckmiss1 strongly depends on the keyword length. So I am curious are there any statistics about length distribution for JS names?

Still it is possible to decrease Ckmiss1 if the patch would first calculate the hash code for scanner char sequence, then do the length switch like the current version but then compare the hash code with precomputed hashcodes of keywords with the matched length.

If there is no match, the code would call a version of string atomizer that takes calculated hash code as an external parameter. 

Comment 12

[Igor Bukanov]

Attachment: Reflection on nits from comment 10

This version is a cleanup to answer nits from comment 10. Instead of making keuword.offset/length uint16 this version removes keyword.length which was a leftover from the initial patch construction.

This version does NOT address the Makefile issue: too much of jet leg for now;).I will do it later together with that precalculated hash code idea.

Comment 13

[Igor Bukanov]

Attachment: Patch with prehash

Here is a version of the patch that first calculate hashcode of the token, then use the hashcode in the generated C code to check for keyword and if no match was found, then it calls js_AtomizeHashedChars which is a version of js_AtomizeChars that take precalculated hashcode.

The version adds a couple of fixes to Makefile.ref: the generated C source is placed into OBJ_DIR and "clean" target now knows about the file.

The patch still does not fix the Makefile.in!

Comment 14

[Igor Bukanov]

Attachment: Patch with prehash (2)

The previous version contained a debug printouts, here is a version without them.

> Here is a version of the patch that first calculate hashcode of the token, then use the hashcode in the generated C code to check for keyword and if no match was found, then it calls js_AtomizeHashedChars which is a version of js_AtomizeChars that take precalculated hashcode.

> The version adds a couple of fixes to Makefile.ref: the generated C source is placed into OBJ_DIR and "clean" target now knows about the file.

> The patch still does not fix the Makefile.in!

Comment 15

[Igor Bukanov]

Attachment: Patch evolution with Makefile.in fixes

Patch changes:

1. Makefile.in is patched so Firefox compiles and runs with the patch applied

2. The patch removes the previous version changes to generate switch over pre-calculated hashcode. It touched too many files without apparent benefits: to minimize cost of keyword miss one can tune switch strategy generation in jskeysw.c.

3. The generated C code use macros to access string length and characters and to do various jumps. In this way it is easy to try to optimize FindKeyword in jsscan.c without changing the source generator.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 206506 [details] [diff] [review]
Patch evolution with Makefile.in fixes

Followup cleanup: js_default_str => const char *const pointer into keyword_chars, and so on for other string constants.

Nit: KEYWORD_COUNT definition: no () around non-type sizeof operands.

Nit: KEYWORD_COUNT definition: align body with KEYWORD_CHARS' body on next line.

test_guess: should use do-while loop, and js_IsKeyword should assert that length is non-zero.

Nit: keyword_chars => chars, to match length and common naming.

JSGEN_ => JSKWGEN_ or JSKW_ to avoid confusion with generators (coming soon) or any other generated name prefix.

Simplify filenames overall by dropping sw after kw: jskwgen.c, jskw.gen, etc.

r=me with followup bug filed and nits picked.  Excellent patch; neat brute-force optimized matcher generator.

/be

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> (From update of attachment 206506 [details] [diff] [review] [edit])
> Followup cleanup: js_default_str => const char *const pointer into
> keyword_chars, and so on for other string constants.

Or(!), avoid extra const-pointer-to-const-char storage by #define js_default_str, etc., to name keyword_chars offsets, and export keyword_chars (js_keyword_chars) to rest of library (but not FRIEND_ or PUBLIC_DATA).  Too bad the C pre-processor is so weak we can't do this without another generated file.

/be

Comment 18

[Erik Fabert]

+ * The Original Code is Mozilla Communicator client code, released
+ * March 31, 1998.
+ *
+ * The Initial Developer of the Original Code is
+ * Netscape Communications Corporation.

This is not true, is it? :-)

Comment 19

[Igor Bukanov]

Attachment: Nitlessing

This version of the patch just addresses nits from above without touching jsopcode.c to share js_default_str etc. chars with keyword_chars. That rather ugly duplicating of macros comes in the next patch version.

Comment 20

[Igor Bukanov]

(In reply to comment #17)
> Or(!), avoid extra const-pointer-to-const-char storage by #define
> js_default_str, etc., to name keyword_chars offsets, and export keyword_chars
> (js_keyword_chars) to rest of library (but not FRIEND_ or PUBLIC_DATA).

Preprocessor trickery to share "default" etc. from keyword_chars touches too many files adding quite a few unfriendly macros in place of js_something_str. Now I think it does not make sense to use string offsets, not pointers, just for the keyword table. 

It is better to apply that approach to all js_something_str. I tried it with a quick-and-dirty patch and it saved 1.5KB from the shell executable which came from reduced number of relocation records for strings. But this is for another enhancement issue, for now I will use string pointers, not offsets in struct keywordin the next version of the patch for this bug.

>  Too
> bad the C pre-processor is so weak we can't do this without another generated
> file.

There is no need for another generated C-file if one simply wants to share C strings without extra relocation records. But it would be nice to generate file that allows to replace
const char js_Object_str[] = "Object";
by
const jschar js_Object_str[] = { 'O', 'b', 'j', 'e', 'c', 't', '\0' };
avoiding the need to malloc memory for JSString.chars representing Object. Yet another bug to file.

Comment 21

[Igor Bukanov]

Attachment: String pointers to keywords.

Here is patch's version that use string pointers in struct keyword to share keyword text with js_default_str, js_new_str etc. 

In the patch I rename js_boolean_str defined in jsatom.h to js_boolean_strs so jsscan.c can define js_boolean_str to represent "boolean". For consistency I also renamed js_type_str to js_type_strs where "strs" means "array of strings".

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 208852 [details] [diff] [review]
String pointers to keywords.

>+static const char js_break_str[]        = "break";

Instead of open-coding these, can't you generate them via KEYWORD macro and #include of jskeyword.tbl?

>+++ src/jskwgen.c
>@@ -0,0 +1,460 @@
>+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-

[snip...]

>+ * The Original Code is Mozilla Communicator client code, released
>+ * March 31, 1998.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):

You should take credit, or give to whomever this came from as far as copyright goes, including years up to 2006.  Original Code can just say "JavaScript Keyword Generator" or some such ;-).

>+++ src/jsconfig.h
>@@ -561,3 +561,8 @@
> #error "unknown JS_VERSION"
> 
> #endif
>+
>+/* Features that present in all versions */
>+#define JS_HAS_RESERVED_JAVA_KEYWORDS   1
>+#define JS_HAS_RESERVED_ECMA_KEYWORDS   1
>+

Comment is missing "are" before "present", and "." at end.

I think HAS_RESERVED_ is a long way of saying RESERVES_, but admit this changes the JS_BUG/JS_HAS pattern.  OTOH the RESERVE_... counterparts from jsscan.c you've moved here used something like that.

r=me with these points addressed.

/be

Comment 23

[Igor Bukanov]

Attachment: Using jskeyword.tbl to define string globals

This version of the patch mostly addresses issue from comment 22. 

The first one about auto_generating js_keyword_str constants required to remove corresponding constant definitions from jsopcode.c and jsatom.c. But this change means that reference to js_private_str used by jsxml.c would not be defined if some one would try to compile SM with JS_HAS_RESERVED_JAVA_KEYWORDS set to 0. But this is purely theoretical, right?

Another patch change is renaming js_incop_str -> js_incop_strs for 
consistency with other renames like that.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 208965 [details] [diff] [review]
Using jskeyword.tbl to define string globals

Here:

>-static struct keyword {
>-    const char  *name;
>+#define JS_KEYWORD(keyword, type, op, version)  \
>+    const char js_##keyword##_str[] = #keyword;
>+#include "jskeyword.tbl"
>+#undef JS_KEYWORD

and here:

>+static const struct keyword keyword_defs[] = {
>+#define JS_KEYWORD(keyword, type, op, version)  \
>+    {js_##keyword##_str, type, op, version},
>+#include "jskeyword.tbl"
>+#undef JS_KEYWORD
>+};

Nit: one space before the backslash, or put the backslash in column 79.  This is a really minor nit, though!

Looking forward to this going in.  Want to sketch the design of the parser and scanner changes in a separate bug, or under http://wiki.mozilla.org/JavaScript ?

/be

Comment 25

[Igor Bukanov]

Attachment: Patch that compiles when objdir != srcdif

The previous version of the patch did not compile on Linux with MOZ_OBJDIR defined to differ from topsrcdir. This version fixes this besides addressing the above macro nit. For consistency with jsautocfg.h I renamed jskw.gen to jsautokw.h. 
Is it possible to verify that the patch compiles on Windows before typing cvs ci?

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 209207 [details] [diff] [review]
Patch that compiles when objdir != srcdif

r+ again, I just checked the interdiff.

/be

Comment 27

[Igor Bukanov]

I committed the last patch.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

This checkin made the windows tinderboxen go red:

/cygdrive/c/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/build/cygwin-wrapper /cygdrive/c/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/build/cygwin-wrapper cl -DMDCPUCFG=\"md/_win95.cfg\" -TC -nologo -Fdjskwgen.pdb -DXP_WIN32 -DXP_WIN -DWIN32 -D_WIN32 -DNO_X11 -Zi -O1 -UDEBUG -DNDEBUG -DOSTYPE=\"WINNT5.0\" -DOSARCH=\"WINNT\" -DBUILD_ID=2006012119 -DEXPORT_JS_API  -DJS_USE_SAFE_ARENA -I../../dist/include/nspr -Fojskwgen.exe jskwgen.c
jskwgen.c
LINK : fatal error LNK1149: output filename matches input filename "c:\builds\tinderbox\MozillaTrunk\WINNT_5.0_Clobber\mozilla\js\src\jskwgen.exe"

Comment 29

[Brendan Eich [:brendan]]

I forgot to offer to help test the patch on Windows, when I get back to work on Monday.  No Windows machines at home atm.  From the look of the error, it should be easy to fix.  Igor, are you around to help?

/be

Comment 30

[:Gavin Sharp [email: [e-mail]]]

I looked into this a bit: OUTOPTION is /Fo on Windows, which is "object file", and "-o" for pretty much everything else (gcc). So for gcc, you get:
gcc -o jskwgen jskwgen.c
which is valid, while for windows/msvc you get:
cl /Fojskwgen.exe jskwgen.c
which is an invalid use of the /Fo flag. Changing OUTOPTION to /Fe, the "executable" flag, (as in
http://lxr.mozilla.org/seamonkey/source/directory/c-sdk/config/Makefile#122
) fixes the bustage. Alternatively, removing the "$(OUTOPTION)$@" works as well, since cl defaults to creating an executable with the same name as the source file, but that would not work for other platforms.

Comment 31

[Brendan Eich [:brendan]]

(In reply to comment #30)
> I looked into this a bit: OUTOPTION is /Fo on Windows, which is "object file",
> and "-o" for pretty much everything else (gcc). So for gcc, you get:
> gcc -o jskwgen jskwgen.c
> which is valid, while for windows/msvc you get:
> cl /Fojskwgen.exe jskwgen.c
> which is an invalid use of the /Fo flag. Changing OUTOPTION to /Fe, the
> "executable" flag, (as in
> http://lxr.mozilla.org/seamonkey/source/directory/c-sdk/config/Makefile#122
> ) fixes the bustage. Alternatively, removing the "$(OUTOPTION)$@" works as
> well, since cl defaults to creating an executable with the same name as the
> source file, but that would not work for other platforms.

I hacked the following into js/src/Makefile.in:

+ifeq (,$(filter-out WINNT WINCE,$(OS_ARCH)))
+# XXXbe See bug 319719 -- mozilla/config/rules.mk HOST_* support has rotted.
+jskwgen$(HOST_BIN_SUFFIX): jskwgen.c jskeyword.tbl Makefile.in
+       $(HOST_CC) $(HOST_CFLAGS) $(DEFINES) $(NSPR_CFLAGS) $<
+else
 jskwgen$(HOST_BIN_SUFFIX): jskwgen.c jskeyword.tbl Makefile.in
        $(HOST_CC) $(HOST_CFLAGS) $(DEFINES) $(NSPR_CFLAGS) $(OUTOPTION)$@ $<
 endif
+endif
 
I hope this fixes things.

It looks like the unconditional definition in mozilla/config/rules.mk of OUTOPTION as -Fo  no longer works with the HOST_PROGRAM and HOST_SIMPLE_PROGRAMS support in that same make-include file.

Someone more clueful about the build system and any changes to the Windows tools over the years should weigh in, maybe file a followup bug (which I really should reference from that comment shown above, instead of this bug).  Thanks.

/be

Comment 32

[Brendan Eich [:brendan]]

And just to be explicit, we should change js/src/Makefile.in to use HOST_PROGRAM or (if necessary; looks like not) HOST_SIMPLE_PROGRAMS once they've been fixed.

/be

Comment 33

[Igor Bukanov]

(In reply to comment #28)
> This checkin made the windows tinderboxen go red:

Sorry about that: I should wait until somebody confirms that non-Linux builds are OK until committing the patch.

Comment 34

[Stefan Sitter [:ssitter]]

Building Sunbird with cygwin/mingw on Windows 2000 from latest trunk cvs (including brendans hack) fails:

/cygdrive/c/dev/moz_sunbird/mozilla/build/cygwin-wrapper gcc -DMDCPUCFG=\"md/_win95.cfg\" -mno-cygwin -DXP_WIN32 -DXP_WIN -DWIN32 -D_WIN32 -DNO_X11 -O -DOSTYPE=\"WINNT5.0\" -DOSARCH=\"WINNT\" -DBUILD_ID=2006012218 -DEXPORT_JS_API  -DJS_USE_SAFE_ARENA -I../../dist/include/nspr /cygdrive/c/dev/moz_sunbird/mozilla/js/src/jskwgen.c

./jskwgen.exe jsautokw.h

make[5]: ./jskwgen.exe: Command not found

make[5]: *** [jsautokw.h] Error 127

Comment 35

[Mike Shaver (:shaver emeritus)]

Brendan's hack seems to assume that Win32 == MSVC, which is indeed not always the case.  Tempted to back the lot out and wait for bsmedberg to review the build changes, so that people can build the trunk with cygwin, rather than trying to guess how to change that without a cygwin build to hand.  Someone convince me otherwise while I get lunch?

Comment 36

[Brendan Eich [:brendan]]

How about going forward rather than backward?  It must be clear to MinGW folks how to solve this, since js/src/Makefile.in is not the only place in the tree where a host-built cross-tool is needed.

/be

Comment 37

[Igor Bukanov]

I reverted the commit since it broke too much.

Comment 38

[cls]

Normally, HOST_PROGRAMS & HOST_SIMPLE_PROGRAMS are only used when cross-compiling. I don't think anyone's successfully cross-compiled mozilla using win32 as the host (targetting wince doesn't count) so they've never been properly tested for win32.  The fix for the common rules, to use -Fe instead of -Fo, should be simple enough and should remove the need for the custom rules in js/src/Makefile.in .

Comment 39

[Brendan Eich [:brendan]]

Hey, cls -- it just occurred to me that even when not cross-compiling, we build and run tools such as jskwgen on the build-host.  Long ago, jscpucfg was run too. I wondered, therefore, whether -Fo used to work when compiling and linking one .exe from one source.

/be

Comment 40

[cls]

Attachment: fix HOST_SIMPLE_PROGRAMS for win32

This patch fixes HOST_SIMPLE_PROGRAMS when building on win32.  You'll need to add HOST_SIMPLE_PROGRAMS = host_jskwgen$(HOST_BIN_SUFFIX) to js/src/Makefile.in.  The host_ prefix is important to trigger the dependency rules in rules.mk. 

Brendan, running jscpucfg.h on win32 predates me.  We've always used the checked in jscpucfg.h for win32 & classic mac instead of the generated jsautocfg.h.

Comment 41

[Igor Bukanov]

(In reply to comment #40)

> You'll need to
> add HOST_SIMPLE_PROGRAMS = host_jskwgen$(HOST_BIN_SUFFIX) to
> js/src/Makefile.in.  The host_ prefix is important to trigger the dependency
> rules in rules.mk. 

rules.mk contains the rule:

ifdef HOST_SIMPLE_PROGRAMS
GARBAGE	+= $(addprefix host_,$(HOST_SIMPLE_PROGRAMS:%=%.$(OBJ_SUFFIX)))
endif

Should it be just 

ifdef HOST_SIMPLE_PROGRAMS
GARBAGE	+= $(HOST_SIMPLE_PROGRAMS:%=%.$(OBJ_SUFFIX))
endif

as HOST_SIMPLE_PROGRAMS alreday include "host_"?


HOST_SIMPLE_PROGRAMS = host_jskwgen$(HOST_BIN_SUFFIX)

> 
> Brendan, running jscpucfg.h on win32 predates me.  We've always used the
> checked in jscpucfg.h for win32 & classic mac instead of the generated
> jsautocfg.h.
> 

Comment 42

[Igor Bukanov]

Attachment: Patch with Makefile.in changes to use HOST_ prefixes/vars.

Comment 43

[cls]

Igor, that's correct.  The extra host_ prefix isn't needed for deleting the intermediate object file.  I checked in the rules.mk patch with that change added.

Checking in config/rules.mk;
/cvsroot/mozilla/config/rules.mk,v  <--  rules.mk
new revision: 3.498; previous revision: 3.497
done

Comment 44

[Igor Bukanov]

Comment on attachment 209446 [details] [diff] [review]
Patch with Makefile.in changes to use HOST_ prefixes/vars.

Asking for review and confirmations that both Mozilla.in and Mozilla.ref changes work on Windows.

Comment 45

[Bob Clary [:bc] (inactive)]

(In reply to comment #44)
making js shell debug/winxp/msvc6

jsscan.c(114) : fatal error C1083: Cannot open include file: 'jsautokw.h': No such file or directory

Comment 46

[Igor Bukanov]

Attachment: Fixing Makefile.ref

Here is the fix for the problem from comment 44: previously -I$(OBJDIR) was included only when using autogenerated config. Would MSVC compile this?

Comment 47

[Bob Clary [:bc] (inactive)]

(In reply to comment #46)
> Created an attachment (id=209466) [edit]
> Fixing Makefile.ref 
> 
> Here is the fix for the problem from comment 44: previously -I$(OBJDIR) was
> included only when using autogenerated config. Would MSVC compile this?
> 
with this patch, make Makefile.ref builds the shell ok, but trying to build Firefox gives:

make[5]: *** No rule to make target `host_jskwgen.exe', needed by `jsautokw.h'.  

Comment 48

[Igor Bukanov]

(In reply to comment #43 by Chris Seawood)
> The extra host_ prefix isn't needed for deleting the intermediate object file. 

Actually that assignment for GARBAGE for HOST_SIMPLE_PROGRAMS (and for SIMPLE_PROGRAMS above it) still does not look right on Windows as it ignores $(HOST_BIN_SUFFIX).

Comment 49

[Igor Bukanov]

(In reply to comment #47 by Bob Clary)
> make Makefile.ref builds the shell ok, but trying to build Firefox gives:
> 
> make[5]: *** No rule to make target `host_jskwgen.exe', needed by `jsautokw.h'. 

Here is the rules that the patch adds to Makefile.in:

HOST_SIMPLE_PROGRAMS += host_jskwgen$(HOST_BIN_SUFFIX)
GARBAGE += jsautokw.h host_jskwgen$(HOST_BIN_SUFFIX)

# Extra dependancies for jsscan.o
jsscan.$(OBJ_SUFFIX): jsautokw.h jskeyword.tbl

jsautokw.h: host_jskwgen$(HOST_BIN_SUFFIX) jskeyword.tbl
	$< $@

Any idea why it does not work with Windows build?

Comment 50

[cls]

ALL_TRASH contains TARGETS which contains HOST_PROGRAMS & HOST_SIMPLE_PROGRAMS.  The rule that I fixed is meant to remove the intermediate object file not the executable.

> HOST_SIMPLE_PROGRAMS += host_jskwgen$(HOST_BIN_SUFFIX)
> GARBAGE += jsautokw.h host_jskwgen$(HOST_BIN_SUFFIX)

Those lines need to be set before rules.mk is included in the makefile.

Comment 51

[Igor Bukanov]

Attachment: Fixing Makefile.in

One more attempt: how does this fire on Windows?

Comment 52

[Bob Clary [:bc] (inactive)]

I think it is ok. debug shell built fine and firefox at least didn't crap out on a compile error in js land although I have linker errors from elsewhere not related to js.

Comment 53

[Igor Bukanov]

Comment on attachment 209523 [details] [diff] [review]
Fixing Makefile.in

Asking for a review: is this necessary formality in this case?

Comment 54

[Stefan Sitter [:ssitter]]

(In reply to comment #53)
> (From update of attachment 209523 [details] [diff] [review] [edit])
> Asking for a review: is this necessary formality in this case?

After applying that patch I get the following error on WinXP:

make[5]: Entering directory `/cygdrive/d/dev/sb-debug/js/src'
Creating .deps
touch jsautocfg.h
/cygdrive/d/dev/mozilla/build/cygwin-wrapper /cygdrive/d/dev/mozilla/build/cygwin-wrapper cl -DMDCPUCFG=\"md/_win95.cfg\" -Fohost_jskwgen.exe -TC -nologo -Fdhost_jskwgen.pdb -DXP_WIN32 -DXP_WIN -DWIN32 -D_WIN32 -DNO_X11   -I../../dist/include/js -I../../dist/include -I../../dist/include/nspr    -I../../dist/sdk/include -I/cygdrive/d/dev/mozilla/js/src host_jskwgen.obj  
host_jskwgen.obj
host_jskwgen.obj(1) : error C2018: unknown character '0x1'
host_jskwgen.obj(1) : error C2018: unknown character '0x5'
host_jskwgen.obj(1) : error C2018: unknown character '0x99'
host_jskwgen.obj(1) : error C2061: syntax error : identifier 'Q'
host_jskwgen.obj(1) : error C2059: syntax error : ';'
host_jskwgen.obj(1) : error C2018: unknown character '0xd7'
host_jskwgen.obj(1) : error C2018: unknown character '0x16'
host_jskwgen.obj(1) : error C2018: unknown character '0x1d'
host_jskwgen.obj(1) : error C2018: unknown character '0xa4'
host_jskwgen.obj(1) : error C2143: syntax error : missing '{' before '.'
host_jskwgen.obj(1) : error C2059: syntax error : '.'
host_jskwgen.obj(1) : error C2018: unknown character '0xdc'
host_jskwgen.obj(2) : error C2018: unknown character '0x10'
host_jskwgen.obj(2) : error C2018: unknown character '0xec'
host_jskwgen.obj(2) : error C2018: unknown character '0x2'
host_jskwgen.obj(2) : error C2018: unknown character '0x1'
host_jskwgen.obj(2) : error C2018: unknown character '0xee'
host_jskwgen.obj(2) : error C2018: unknown character '0x1'
host_jskwgen.obj(2) : error C2018: unknown character '0x40'
host_jskwgen.obj(2) : error C2059: syntax error : 'constant'
host_jskwgen.obj(2) : error C2018: unknown character '0x40'
host_jskwgen.obj(2) : error C2018: unknown character '0x5'
host_jskwgen.obj(2) : error C2018: unknown character '0x40'
host_jskwgen.obj(2) : error C2018: unknown character '0xc0'
host_jskwgen.obj(2) : error C2018: unknown character '0xa8'
host_jskwgen.obj(2) : error C2018: unknown character '0xf4'
host_jskwgen.obj(2) : error C2018: unknown character '0x16'
host_jskwgen.obj(2) : error C2018: unknown character '0x9d'
host_jskwgen.obj(2) : error C2018: unknown character '0x60'
host_jskwgen.obj(2) : error C2018: unknown character '0x5'
host_jskwgen.obj(2) : error C2018: unknown character '0x80'
host_jskwgen.obj(2) : error C2018: unknown character '0xc0'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x8'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x10'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x14'
host_jskwgen.obj(2) : error C2018: unknown character '0x40'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x18'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x1c'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2059: syntax error : ','
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x40'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2017: illegal escape sequence
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x60'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2059: syntax error : ')'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : error C2018: unknown character '0x6'
host_jskwgen.obj(2) : fatal error C1004: unexpected end of file found
make[5]: *** [host_jskwgen.exe] Error 2

Comment 55

[Igor Bukanov]

(In reply to comment #54)
> After applying that patch I get the following error on WinXP:

Did you update config/rules.mk from CVS? A fix from Chris was supposed to address this.

Comment 56

[Stefan Sitter [:ssitter]]

(In reply to comment #55)
> Did you update config/rules.mk from CVS? A fix from Chris was supposed to
> address this.

Thanks, I missed that. After update my win32/cygwin/mingw build was successful.

Comment 57

[Brendan Eich [:brendan]]

Comment on attachment 209523 [details] [diff] [review]
Fixing Makefile.in

Looks fine to me, but again if we can get cls's rules.mk patch in, why not do that and minimize this patch's Makefile.in changes?

/be

Comment 58

[Igor Bukanov]

(In reply to comment #57)
> (From update of attachment 209523 [details] [diff] [review] [edit])
> ... if we can get cls's rules.mk patch in, why not do
> that and minimize this patch's Makefile.in changes?

That patch from Chris has been already committed and taken into account in the last attachment 209523 [details] [diff] [review]: there was no platform-specific code there.

Comment 59

[Brendan Eich [:brendan]]

Comment on attachment 209523 [details] [diff] [review]
Fixing Makefile.in

Sorry, I'm blind -- I read right across the Makefile.ref diff boundary! Anyway, you've been +'d already :-).

/be

Comment 60

[Igor Bukanov]

Attachment: Patch to commit

Comment 61

[Igor Bukanov]

I committed the last patch

Comment 62

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #61)
> I committed the last patch

That checkin has caused Linux/Mac bustage on the SeaMonkey and Firefox trees.

gmake[5]: Entering directory `/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src'
host_jskwgen jsautokw.h
gmake[5]: host_jskwgen: Command not found

Comment 63

[Igor Bukanov]

I did (In reply to comment #62)
> That checkin has caused Linux/Mac bustage on the SeaMonkey and Firefox trees.
> 
> gmake[5]: Entering directory
> `/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src'
> host_jskwgen jsautokw.h
> gmake[5]: host_jskwgen: Command not found

I should learn to prefix ./ when invoking commands in the current directory. I had PATH with "." so that is why it compiled on my Linux box. I will commit the following patch to fix this withnthe hope that ./something would also work on Windows.

Index: Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/js/src/Makefile.in,v
retrieving revision 3.100
diff -U7 -r3.100 Makefile.in
--- Makefile.in 26 Jan 2006 08:47:50 -0000      3.100
+++ Makefile.in 26 Jan 2006 10:27:44 -0000
@@ -398,8 +398,8 @@
 endif
 endif
 
 # Extra dependancies and rules for keyword switch code
 jsscan.$(OBJ_SUFFIX): jsautokw.h jskeyword.tbl
 
 jsautokw.h: host_jskwgen$(HOST_BIN_SUFFIX) jskeyword.tbl
-       $< $@
+       ./host_jskwgen$(HOST_BIN_SUFFIX) $@

Comment 64

[Igor Bukanov]

Attachment: Total patch with the last Makefile.in change included

Here is for references an accumulated patch that includes the last one line fix.