link transformiix standalone to XPCOM

VERIFIED FIXED

Status

()

VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: axel, Assigned: axel)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 15 obsolete attachments)

52.91 KB, application/gzip
Details
60.14 KB, application/gzip
Details
(Assignee)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
oh, a new configure ain't all that easy, too. As we need 
toplevel/config/autoconf.mk
(Assignee)

Comment 2

16 years ago
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.
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.

(Assignee)

Comment 4

16 years ago
Created attachment 107297 [details] [diff] [review]
do build, strings and logging with xpcom/nspr for standalone
(Assignee)

Comment 5

16 years ago
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.
Assignee: peterv → axel
(Assignee)

Comment 6

16 years ago
... 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.
(Assignee)

Comment 7

16 years ago
Created attachment 107299 [details]
TxString.cpp
(Assignee)

Comment 8

16 years ago
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)
(Assignee)

Comment 9

16 years ago
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.
(Assignee)

Comment 10

16 years ago
Created attachment 108384 [details]
gprof output for standalone trunk (gzipped)
(Assignee)

Comment 11

16 years ago
Created attachment 108385 [details]
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)
(Assignee)

Comment 12

16 years ago
Created attachment 108386 [details] [diff] [review]
patch building on windows, linux. Better use of iterators in XMLUtils::isWhiteSpace
(Assignee)

Updated

16 years ago
Attachment #107297 - Attachment is obsolete: true
I think XMLUtils::isWhiteSpace should take nsAFlatString and use
nsAFlatString::const_iterator
(Assignee)

Updated

16 years ago
Blocks: 184086
(Assignee)

Updated

16 years ago
Blocks: 74786
(Assignee)

Comment 14

16 years ago
Created attachment 109033 [details] [diff] [review]
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.
Attachment #108386 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #109033 - Flags: superreview?(peterv)
Attachment #109033 - Flags: review?(bugmail)
(Assignee)

Comment 15

16 years ago
Created attachment 109518 [details] [diff] [review]
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.
Attachment #109033 - Attachment is obsolete: true
(Assignee)

Comment 16

16 years ago
Created attachment 109519 [details]
TxString.cpp for your reviewing pleasure
Attachment #107299 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #109518 - Flags: superreview?(peterv)
Attachment #109518 - Flags: review?(bugmail)
(Assignee)

Updated

16 years ago
Attachment #109033 - Flags: superreview?(peterv)
Attachment #109033 - Flags: review?(bugmail)
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
Attachment #109518 - Flags: review?(bugmail) → review+
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.
Attachment #109518 - Flags: superreview?(peterv) → superreview+
(Assignee)

Comment 19

16 years ago
> 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.
Status: NEW → ASSIGNED
(Assignee)

Comment 20

16 years ago
Created attachment 109684 [details] [diff] [review]
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 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
(Assignee)

Comment 22

16 years ago
Created attachment 109770 [details] [diff] [review]
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.
(Assignee)

Updated

16 years ago
Attachment #109684 - Attachment is obsolete: true
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.
Attachment #109770 - Flags: superreview+
Attachment #109770 - Flags: review?(bugmail)
(Assignee)

Comment 24

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

done locally
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
Attachment #109770 - Flags: review?(bugmail) → review+
(Assignee)

Comment 26

16 years ago
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
(Assignee)

Comment 28

16 years ago
Created attachment 111837 [details] [diff] [review]
remove standalone parts which have nspr code path

we can use the same nspr functions like module now
Comment on attachment 111836 [details] [diff] [review]
remove txAtom typedef

got rs=peterv on irc
Attachment #111836 - Flags: superreview+
(Assignee)

Comment 30

16 years ago
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>
Attachment #111836 - Flags: review+
(Assignee)

Comment 31

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

I ran the xalan tests, all fine.
Attachment #111837 - Flags: superreview?(peterv)
Comment on attachment 111836 [details] [diff] [review]
remove txAtom typedef

checked in
Attachment #111836 - Attachment is obsolete: true
Created attachment 111888 [details] [diff] [review]
remove txAtom.h
Attachment #109518 - Attachment is obsolete: true
Attachment #109519 - Attachment is obsolete: true
Attachment #109770 - Attachment is obsolete: true
Attachment #111888 - Flags: superreview?(peterv)
Attachment #111888 - Flags: review?(axel)
(Assignee)

Comment 34

16 years ago
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=axel@pike.org, assuming you ran buster
and MOZ_DUMP_ATOM_LEAKS
Attachment #111888 - Flags: review?(axel) → review+
> >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.
(Assignee)

Comment 36

16 years ago
Created attachment 111971 [details] [diff] [review]
clean up old TX_EXE specific configure tests, allmakefiles.sh; get debug symbols back on win standalone
(Assignee)

Comment 37

16 years ago
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.
Attachment #111971 - Flags: review?(cls)
Attachment #111837 - Flags: superreview?(peterv) → superreview+
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?
Attachment #111888 - Flags: superreview?(peterv) → superreview+
Created attachment 112001 [details] [diff] [review]
better txStack and remove NamedMap for documents
Attachment #112001 - Flags: review?(bugmail)
(Assignee)

Comment 40

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

checked in
Attachment #111837 - Attachment is obsolete: true
Comment on attachment 111888 [details] [diff] [review]
remove txAtom.h

checked in. Yes IMHO we should make it take an nsIAtom** instead
Attachment #111888 - Attachment is obsolete: true
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
Attachment #111971 - Flags: review?(cls) → review+
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?
Attachment #112001 - Flags: review?(bugmail) → review-
Created attachment 112043 [details] [diff] [review]
better txStack and remove NamedMap for documents
Attachment #112001 - Attachment is obsolete: true
Attachment #112043 - Flags: review?(bugmail)
Created attachment 112045 [details] [diff] [review]
move resolveQName to nsIAtom**
Comment on attachment 112043 [details] [diff] [review]
better txStack and remove NamedMap for documents

r=sicking
Attachment #112043 - Flags: review?(bugmail) → review+
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.
(Assignee)

Comment 49

16 years ago
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
Attachment #111971 - Attachment is obsolete: true
Attachment #112043 - Flags: superreview?(jst)
Attachment #112045 - Flags: superreview?(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
Attachment #112043 - Flags: superreview?(jst) → superreview+
Comment on attachment 112045 [details] [diff] [review]
move resolveQName to nsIAtom**

sr=jst
Attachment #112045 - Flags: superreview?(jst) → superreview+
Comment on attachment 112043 [details] [diff] [review]
better txStack and remove NamedMap for documents

Checked in.
Attachment #112043 - Attachment is obsolete: true
Comment on attachment 112045 [details] [diff] [review]
move resolveQName to nsIAtom**

Checked in.
Attachment #112045 - Attachment is obsolete: true
(Assignee)

Comment 54

16 years ago
marking fixed, we have separate bugs for the remaining stuff now.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 55

16 years ago
mass verifying
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.