157142 - link transformiix standalone to XPCOM

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Axel Hecht]

We would like to get rid of some of our duplicate work for transformiix
standalone, and the cleanest way is to pull in XPCOM (and nspr).
The ability to use XPCOM, and mozilla strings is a big pro, so let's deal with
the cons.

Source size:
7643    nsprpub (or 12016, I kinda get two different numbers, no idea why)
11782   xpcom
1063    string
2311    extensions/transformiix

If we had a nspr-config, we could use --with-system-nspr, but that's broken.
We might be able to use mozilla-config, though. That would require us to
roll our own configure, I guess. Didn't manage to tweak mozilla/configure 
and the rest of the gang to play nice with it.

Or do we just want to bite the bullet? I'm not too keen, considering the bunch
of trees we have. 33M of additional source, I'm almost ready to hit another
configure.in. I guess we could hack something for windows, too, not sure what
MRE or installed builds look like there.

Comment 1

[Axel Hecht]

oh, a new configure ain't all that easy, too. As we need 
toplevel/config/autoconf.mk

Comment 2

[Axel Hecht]

I tried to get nspr/xpcom out by using 
mk_add_options BUILD_MODULES="xpcom transformiix"
ac_add_options --enable-standalone-modules="xpcom transformiix"

this leaves the source with xpcom/nspr at 26546k instead of 5648k. Not sure what
the other numbers were.

I have one issue, it doesn't work. I get the following:

cvs server: warning: new-born mozilla/allmakefiles.sh has disappeared
cvs server: warning: new-born mozilla/aclocal.m4 has disappeared
cvs server: warning: new-born mozilla/configure has disappeared
cvs server: warning: new-born mozilla/configure.in has disappeared
cvs server: warning: new-born mozilla/Makefile.in has disappeared

I'm pretty amazed, cls, any idea?
I worked around it by just removing mozilla/CVS/Entries.Static and cvs co those 
files. cvs update didn't do it.
That workaround is just not cutting edge :-(

After fixing that, we need to get strings and xpcom linking in one step, as 
we get linking conflicts between xpcom and our mockups. Esp.
NS_ConvertASCIIToUCS2. I get linking conflicts between xpcom and nspr, not sure
how badly to worry on those.

Comment 3

[hacker formerly known as [e-mail]]

I have no idea how you're coming up with these numbers but you missed
modules/libreg .  

The cvs checkout problem was first noticed after cvs.m.o was upgraded to 1.11.x.
 I hit it a handful of times but I haven't seen it since.  See bug 122149.
  What version of the cvs client are you using?

Hrm. This entire standalone but not standalone business is starting to become
annoying again.  Why does the standalone transformiix module need to be tied
into the toplevel build system at all since the standalone version does not
distribute the same module that the non-standalone version does?  It seems like
transformiix should be taking the JS approach of having a separate build system
for the non-Mozilla releases of the module.  

You could have your own configure system for the standalone build and reuse the
 existing Makefiles.  You'd have to duplicate all of the autoconf tests and
makefile rules in your local system.  Depending upon what the actual goal of the
standalone build is, it may or may not be worth the trouble.

Comment 4

[Axel Hecht]

Attachment: do build, strings and logging with xpcom/nspr for standalone

Comment 5

[Axel Hecht]

cls: The major difference between the mozilla module and transformiix standalone
is the XML content model. This lets us test the XSLT engine without carrying the
whole lizard, a bit like viewer-extreme :-).
It used to be code independent of Mozilla, for MITRE reasons.
We want to cut down the special casings within transformiix as well as duplicated
code these days, both to ease development and to boost performance.
transformiix standalone already uses mozilla/expat (without the MOZILLA_CLIENT
define) and the build system from SeaMonkey. I have a hard time imagining 
replicating the build system and pulling in other stuff.
So if it's somehow bearable, I'd like to keep us using the global build stuff.

This patch introduces bare standalone REQUIRES lines, not sure what the stuff 
in tools/module-deps will do to it. (It may acutally remove the special casing
for tx and just pull xpcom, nspr and strings, as intended)

I built this with
TX_EXE=1
mk_add_options BUILD_MODULES="xpcom transformiix"
ac_add_options --enable-standalone-modules="xpcom transformiix"

and not setting NSPR_NATIVE (in contrast to the standalone build docs)

This patch makes us link xpcom, nspr and uses xpcom strings with the 
txMozillaString.h wrapper, slightly modified to avoid unicharutils.
unicharutils have huge dependencies, and I don't see myself nukin' those.
I got nspr logging working, too. Now standalone can use NSPR_LOG_MODULES just
like module does. Of course, I removed error code and assertion mockups.

Next, and probably different bug, would be getting rid of txAtom.

I'll attach a version of TxString.cpp, as that diff is unreadable.

Comment 6

[Axel Hecht]

... forgot to say that the cvs error is highly version dependent. On both
server and client.
Thanx to cls for verifying this.
If you get the error, cvs co the files alone. co, update doesn't cut it.
Didn't verify if you *have to* remove mozilla/CVS/Entries.Static, I did.

Comment 7

[Axel Hecht]

Attachment: TxString.cpp

Comment 8

[Axel Hecht]

I tried to remove the standalone atom impl, but that needs some work in
txHTMLOutput.cpp, as that needs TxObject atoms. So I put that off into the 
next stage.
I see a pretty bad slowdown, though. We should measure what this is.
(note that program startup isn't measured, so it shouldn't be code loading)

Comment 9

[Axel Hecht]

XMLUtils::isWhiteSpace(String&) pops up prominently in the profile.
Basically, charAt is slow in module, as it calls into string fragment code.
I hope that I can get some speedup by using iterators.

Comment 10

[Axel Hecht]

Attachment: gprof output for standalone trunk (gzipped)

Comment 11

[Axel Hecht]

Attachment: gprof output for xpcom'd standalone

the two profiles have been created with -NDEBUG and -g -pg, no optims.
Mozilla strings don't perform well, and we don't use them right.
I'll attach a patch in a minute, that builds on linux, too.

IMHO we should land as is, slow down by 2 or so and work on getting the 
performance issues resolved, as they hit our primary target module just as they

hit xpcom'ed standalone.
(the profiles were 100 runs of the XSLTMark tower test)

Comment 12

[Axel Hecht]

Attachment: patch building on windows, linux. Better use of iterators in XMLUtils::isWhiteSpace

Comment 13

[Peter Van der Beken [:peterv]]

I think XMLUtils::isWhiteSpace should take nsAFlatString and use
nsAFlatString::const_iterator

Comment 14

[Axel Hecht]

Attachment: xmlutils should keep String in the interface for now.

I changed the interface of XMLUtils::isWhitespace back to String, but work
on nsAFlatString internally. Conversion thru cast operators didn't work, so
we will have to clean up all the interfaces in one pass. see the string bug for

this.
I built standalone on linux and windows, and module on windows.
Minor change in performance for module, but in the range of noise, due to the
isWhitespace change. This is probably depending on whether the text nodes 
have lots of whitespace stripping or not. As a single charAt just does a 
BeginReading, and this version does a EndReading too. So as long as 
isWhitespace returns on the first  char, it might be a tad cheaper, otherwise
it really shouldn't be.

Comment 15

[Axel Hecht]

Attachment: better code in TxString.cpp

I fixed the case functions to be more Mozilla string.
there is a string comparator for case insensitive compares ifdef TX_EXE now,
which is a typedef to the unicharutils one for module.
I changed the iterators in the case conversion routines to 
nsAFlatString::char_iterators, too.

Comment 16

[Axel Hecht]

Attachment: TxString.cpp for your reviewing pleasure

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 109518 [details] [diff] [review]
better code in TxString.cpp

Way cool dude! this rocks!

Do we need to have both MozillaString.h and TxString.h? Though i'd happily see
them merge as a separate patch. Hmm.. though i guess we should move toward
killing the both of them?

Couldn't even find a nit :-) r=me

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 109518 [details] [diff] [review]
better code in TxString.cpp

Index: source/base/TxString.h
===================================================================

 class String
-#ifdef TX_EXE
 : public TxObject
-#endif

Do we really want to do that? I think we want to eventually remove this
inheritance in standalone too.

Index: source/xml/XMLUtils.cpp
===================================================================

 MBool XMLUtils::isWhitespace(const String& aText)
 {

+    const nsAFlatString& text = aText.getConstNSString();
+    nsAFlatString::const_char_iterator start, end;
+    text.BeginReading(start);
+    text.EndReading(end);
+    for ( ; start != end; start++) {

I'd write this with a while but to each his own. Please use ++start though.

Comment 19

[Axel Hecht]

> Do we really want to do that? I think we want to eventually remove this
> inheritance in standalone too.

I see the fate of String in each star of christmas decoration, so I don't
worry about that inheritance. But if we fix CommandLineUtils, it can probably
go soon

> Index: source/xml/XMLUtils.cpp
> ===================================================================
> 
>  MBool XMLUtils::isWhitespace(const String& aText)
<...>
> I'd write this with a while but to each his own. Please use ++start though.
done.

Checked in, leaving the bug open to do txAtom removal, and use of nspr
functions for standalone.

Comment 20

[Axel Hecht]

Attachment: make standalone use XPCOM atoms

this patch kills the standalone atom implementation.
Performance numbers vary, I hardly see any impact on my windows builds, but 
linux profiling build looked much better. From 134 seconds to 90. No optims
for profiling and linux transforms to /dev/null.

I had to give a beating to txHTMLOutput, it used that txAtoms inherited
TxObject.
It's full of xpcom/ds now. nsCOMPtr and nsCOMArrays. Hey.

I noticed that xpath string functions starts-with and contains worked 
differently in module and standalone. if the second argument is empty, module
says not found, xml-xalan tests think it should be true though.
tests string48,49,61,62.

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 109684 [details] [diff] [review]
make standalone use XPCOM atoms

>Index: source/main/transformiix.cpp
>===================================================================

>@@ -123,7 +127,8 @@
>     }
>     resultFileStream.close();
>     txXSLTProcessor::txShutdown();
>-    return NS_SUCCEEDED(rv);
>+    rv = NS_ShutdownXPCOM(nsnull);
>+    return rv;

return NS_ShutdownXPCOM(nsnull);


>Index: source/xslt/txHTMLOutput.cpp
>===================================================================

>@@ -42,122 +42,83 @@
> #include "XMLUtils.h"
> 
> txHTMLOutput::txHTMLOutput(txOutputFormat* aFormat, ostream* aOut)
>-    : txXMLOutput(aFormat, aOut)
>+    : txXMLOutput(aFormat, aOut), mHTMLEmptyTags(13)

#define this number

Comment 22

[Axel Hecht]

Attachment: make standaleon use XPCOM atoms, with peterv's comments

I #defined the second constant, and changed txXMLOutput to convert to UTF8
instead
of lossy ascii.
I kept the singular line for NS_ShutdownXPCOM, I like to debug there.

Comment 23

[Peter Van der Beken [:peterv]]

Comment on attachment 109770 [details] [diff] [review]
make standaleon use XPCOM atoms, with peterv's comments

>Index: source/xslt/txHTMLOutput.cpp
>===================================================================

>+    mHTMLEmptyAttributes[0].mAttrName = txHTMLAtoms::checked;
>+    mHTMLEmptyAttributes[0].mElementList.AppendObject(txHTMLAtoms::input);

Here you set mAttrName first.

>     // compact
>+    mHTMLEmptyAttributes[1].mElementList.AppendObject(txHTMLAtoms::dir);
>+    mHTMLEmptyAttributes[1].mElementList.AppendObject(txHTMLAtoms::dl);
>+    mHTMLEmptyAttributes[1].mElementList.AppendObject(txHTMLAtoms::menu);
>+    mHTMLEmptyAttributes[1].mElementList.AppendObject(txHTMLAtoms::ol);
>+    mHTMLEmptyAttributes[1].mElementList.AppendObject(txHTMLAtoms::ul);
>+    mHTMLEmptyAttributes[1].mAttrName = txHTMLAtoms::compact;

And here last, please be consistent. I prefer it if you set mAttrName first.

Comment 24

[Axel Hecht]

> And here last, please be consistent. I prefer it if you set mAttrName first.

done locally

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 109770 [details] [diff] [review]
make standaleon use XPCOM atoms, with peterv's comments

>Index: source/main/transformiix.cpp
>-    return NS_SUCCEEDED(rv);
>+    rv = NS_ShutdownXPCOM(nsnull);
>+    return rv;

A success-value can be != 0 so do NS_ENSURE_SUCCESS(rv, rv); return 0;


>Index: source/xslt/txHTMLOutput.cpp
>+    nsCOMPtr<nsIAtom> localAtom = do_GetAtom(localName);
>+    PRUint8 k = 0;
>+    for ( ; k < SHORTHAND_ATTR_COUNT; ++k) {
>+        if (mHTMLEmptyAttributes[k].mAttrName == localAtom) {
>+            txExpandedName* currentElement =
>+                (txExpandedName*)mCurrentElements.peek();
>+            if (mHTMLEmptyAttributes[k].mElementList.IndexOf...
>+                return MB_TRUE;
>+            }

put an |return MB_FALSE| after there. Or simply return the if-test.

>Index: source/xslt/txXMLOutput.cpp
>-            *mOut << *(att->mName.mLocalName);
>+            const PRUnichar* attrVal;
>+            att->mName.mLocalName->GetUnicode(&attrVal);
>+            // XXX consult the XML spec what we really wanna do here
>+            *mOut << NS_ConvertUCS2toUTF8(nsDependentString(attrVal)).get();

i think this should use printUTF8Chars, but peterv tells me that there was some
reason why that couldn't be done until we've fixed some other stuff

Comment 26

[Axel Hecht]

Comment on attachment 109770 [details] [diff] [review]
make standaleon use XPCOM atoms, with peterv's comments

this part of the bug landed and sticked to the tree.
Leaving bug open to carry the nspr ifdefs removal

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: remove txAtom typedef

Comment 28

[Axel Hecht]

Attachment: remove standalone parts which have nspr code path

we can use the same nspr functions like module now

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 111836 [details] [diff] [review]
remove txAtom typedef

got rs=peterv on irc

Comment 30

[Axel Hecht]

Comment on attachment 111836 [details] [diff] [review]
remove txAtom typedef

we should keep in mind to kill the macros and to move some of the nsIAtom* to
nsCOMPtr<nsIAtom>

Comment 31

[Axel Hecht]

Comment on attachment 111837 [details] [diff] [review]
remove standalone parts which have nspr code path

I ran the xalan tests, all fine.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 111836 [details] [diff] [review]
remove txAtom typedef

checked in

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: remove txAtom.h

Comment 34

[Axel Hecht]

Comment on attachment 111888 [details] [diff] [review]
remove txAtom.h

>Index: source/base/txExpandedNameMap.cpp
make mLocalName a nsCOMPtr<nsIAtom>

>Index: source/xpath/txNodeTypeTest.cpp
<...>
>@@ -64,13 +63,10 @@
>                    !aContext->isStripSpaceAllowed(aNode);
>         case PI_TYPE:
>             if (type == Node::PROCESSING_INSTRUCTION_NODE) {
>-                nsIAtom* localName = 0;
>-                MBool result;
>-                result = !mNodeName ||
>-                         (aNode->getLocalName(&localName) &&
>-                          localName == mNodeName);
>-                TX_IF_RELEASE_ATOM(localName);
>-                return result;
>+                nsCOMPtr<nsIAtom> localName = 0;

don't init twice

>+                return !mNodeName ||
>+                        (aNode->getLocalName(getter_AddRefs(localName)) &&
>+                         localName == mNodeName);
>             }
>             return MB_FALSE;
>         case NODE_TYPE:

With those comments, r=[e-mail], assuming you ran buster
and MOZ_DUMP_ATOM_LEAKS

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> >Index: source/base/txExpandedNameMap.cpp
> make mLocalName a nsCOMPtr<nsIAtom>

I can't do that since we would release all atoms when we allocate a new array
and delete the old one.

Comment 36

[Axel Hecht]

Attachment: clean up old TX_EXE specific configure tests, allmakefiles.sh; get debug symbols back on win standalone

Comment 37

[Axel Hecht]

Comment on attachment 111971 [details] [diff] [review]
clean up old TX_EXE specific configure tests, allmakefiles.sh; get debug symbols back on win standalone

due to having nspr/xpcom, we don't need special treatment in configure.in, we'd
just like to keep the AC_DEFINE(TX_EXE).
The makefile stuff is to build the lib independent of the SIMPLE_PROGRAMS, as
PDFILE conflicts.

Comment 38

[Peter Van der Beken [:peterv]]

Comment on attachment 111888 [details] [diff] [review]
remove txAtom.h

>Index: source/xpath/ExprParser.cpp
>===================================================================

>@@ -326,17 +326,16 @@
>             break;
>         case Token::VAR_REFERENCE :
>             {
>-                nsIAtom *prefix, *lName;
>+                nsCOMPtr<nsIAtom> prefix, lName;
>                 PRInt32 nspace;
>-                nsresult rv = resolveQName(tok->value, prefix, aContext,
>-                                           lName, nspace);
>+                nsresult rv = resolveQName(tok->value, *getter_AddRefs(prefix),
>+                                           aContext,
>+                                           *getter_AddRefs(lName), nspace);

Make this:

		nsresult rv = resolveQName(tok->value, *getter_AddRefs(prefix),
					   aContext, *getter_AddRefs(lName),
					   nspace);


Should we make resolveQName take nsIAtom**'s?

Comment 39

[Peter Van der Beken [:peterv]]

Attachment: better txStack and remove NamedMap for documents

Comment 40

[Axel Hecht]

Comment on attachment 111837 [details] [diff] [review]
remove standalone parts which have nspr code path

checked in

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 111888 [details] [diff] [review]
remove txAtom.h

checked in. Yes IMHO we should make it take an nsIAtom** instead

Comment 42

[hacker formerly known as [e-mail]]

Comment on attachment 111971 [details] [diff] [review]
clean up old TX_EXE specific configure tests, allmakefiles.sh; get debug symbols back on win standalone

r=cls

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 112001 [details] [diff] [review]
better txStack and remove NamedMap for documents

>Index: source/base/txStack.h

>+    txStack()
>+    {
>+    }
[snip]
>+    ~txStack()
>+    {
>+    }

IMHO you don't need these.

>+    void push(void* aObject)
>+    {
>+        AppendElement(aObject);
>+    }

please make this function return an nsresult. We'll need that anyway.

>+    void* pop()
>+    {
>+        PRInt32 count = Count() - 1;
>+        void* object = ElementAt(count);
>+        if (RemoveElementAt(count)) {
>+            return object;
>+        }
>+        return nsnull;
>+    }

The only way RemoveElementAt can return false is if you call it with a bad
index. And by then we should have crashed anyway since ElementAt isn't
out-of-bounds-safe (which is ok since the old pop() didn't support popping from
any empty stack either). So remove the |if| and always return object.

IMHO it's ok to keep this thing inlined.


hmmmm.. i'm not all that happy about the txLoadedDocumentsHash-class. IMHO
that's a whole lot of code to get around two hash-lookups per transformation.
Or does it do anything else that i'm missing?

Comment 44

[Peter Van der Beken [:peterv]]

Attachment: better txStack and remove NamedMap for documents

Comment 45

[Peter Van der Beken [:peterv]]

Attachment: move resolveQName to nsIAtom**

Comment 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 112043 [details] [diff] [review]
better txStack and remove NamedMap for documents

r=sicking

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 112043 [details] [diff] [review]
better txStack and remove NamedMap for documents

forgot to say: feel free to remove the |if (mXDocument)|s in the ctor/dtor.
They should never be null anyway.

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 112045 [details] [diff] [review]
move resolveQName to nsIAtom**

r=me

Comment 49

[Axel Hecht]

Comment on attachment 111971 [details] [diff] [review]
clean up old TX_EXE specific configure tests, allmakefiles.sh; get debug symbols back on win standalone

checked in

Comment 50

[Johnny Stenback (:jst)]

Comment on attachment 112043 [details] [diff] [review]
better txStack and remove NamedMap for documents

- In txStack.h:

+    void* peek()
+    {
+	 return ElementAt(Count() - 1);

From what I can tell, nsVoidArray::ElementAt() is not safe against negative
indexes (yet it takes a signed argument...), IOW, either check here, or make
nsVoidArray suck less. Same thing in pop().

+class txStackIterator
...
+    PRUint32 mCounter;

From looking at how this is used, it looks more like a position to me than a
counter. IOW, rename to mPosition, or something like that.

- In txLoadedDocumentsHash::txLoadedDocumentsHash():

+    PRBool isLive = Init(8);

Shouldn't isLive be an nsresult here?

+    if (!isLive) {
+	 mHashTable.ops = nsnull;

And it looks to me like this is already done for you if the hash can't be
init'ed.

Other than that, sr=jst

Comment 51

[Johnny Stenback (:jst)]

Comment on attachment 112045 [details] [diff] [review]
move resolveQName to nsIAtom**

sr=jst

Comment 52

[Peter Van der Beken [:peterv]]

Comment on attachment 112043 [details] [diff] [review]
better txStack and remove NamedMap for documents

Checked in.

Comment 53

[Peter Van der Beken [:peterv]]

Comment on attachment 112045 [details] [diff] [review]
move resolveQName to nsIAtom**

Checked in.

Comment 54

[Axel Hecht]

marking fixed, we have separate bugs for the remaining stuff now.

Comment 55

[Axel Hecht]

mass verifying