Closed GoogleCodeExporter closed 9 years ago
This looks good and I'm going to import it into the 17 changesets. Tobias, any
objections?
So the real question now is builds based upon it. Clearly I can't support them
or load them onto build.floodgap.com personally because I don't use x86, but
there is an interest, and I suppose we could make these builds "unsupported"
(no Tenderapp issues are accepted on them and the what's new page, if it sees
Intel Mac OS X, will display a warning). However, I am happy to *host* them if
someone(tm) is willing to keep up with builds.
Tentatively assigning to me for tracking purposes.
Original comment by classi...@floodgap.com
on 23 Jan 2013 at 3:52
Actually, there is one thing. It doesn't look like you're using the OSAtomics
for JS, which probably accounts for the slower benchmarks you were seeing.
There is a native OSAtomic for i386, though you'd need to write an atomic set
routine (my x86 is rusty). It's not a critical thing but it would probably
restore most of the speed deficit.
Original comment by classi...@floodgap.com
on 23 Jan 2013 at 3:59
Well, I'll volunteer to be that someone(tm) on a best-effort basis. That is,
I'm happy to do it but I can't guarantee super-duper timeliness. The same
should apply for bug fixes and the like.
Thanks for the tip on native OSAtomic.
Original comment by lei...@gmail.com
on 23 Jan 2013 at 6:17
I've upgraded you to be able to upload to the Downloads tab. Note that for
downloadable binaries there's a naming and description convention which I'm
sure you've seen. Don't worry about uploading the changeset; I'll take care of
that with 17.0.3 and the interested can get it from here.
For the atomics, these apply to Intel/i386 also:
#define PR_ATOMIC_ADD(p,v) OSAtomicAdd32(v,p)
extern PRInt32 OSAtomicAdd32(PRInt32 v, PRInt32 *p);
/* Increment and Decrement are just implemented in terms of Add. */
#define PR_ATOMIC_INCREMENT(p) PR_ATOMIC_ADD(p,1)
#define PR_ATOMIC_DECREMENT(p) PR_ATOMIC_ADD(p,-1)
For the atomic set, it's a locked exchange (xchgl). We could still call it
TenFourFoxAtomicSet and have an #ifdef _ppc_ and #else block so that way we
need only modify darwin.S, or we could rig it to use __PR_Darwin_x86_AtomicSet.
In fact, I'm thinking of converting the PPC routines over to just use the _PR
ones after all because the __PR* ones don't have any sync and we already know
from TenFourFoxAtomicSet that it isn't necessary. That can be a future patch
though; I'll open a new issue for that. For PPC I'll probably do it on unstable
to start.
I'm also going to write up a wiki page so that people don't get the wrong idea
about the support level of the Intel build.
Is crediting you as Claudio Leite fine (in the About box)?
Original comment by classi...@floodgap.com
on 24 Jan 2013 at 1:15
Yes, that is fine for the credits.
I'm stuck at work for another few hours so I won't have a chance until tomorrow
night to work on the atomic ops. I should be able to figure it out based on
what you've laid out. Thanks for the help.
Do you have a patch set that applies cleanly against what's coming out of
mozilla-esr17 hg? I've been using the 17.0.2 patch set and ended up applying it
against the tarball.
Original comment by lei...@gmail.com
on 24 Jan 2013 at 1:44
No, not yet. Generally I wait until the Friday or Saturday before release day,
pull from hg since it is usually finalized and then merge the changesets at
that point since stuff is always landing. This is the release drivers calendar:
https://mail.mozilla.com/home/akeybl@mozilla.com/Release%20Management.html
The atomics don't need to make it into this version, but see issue 205. The
only other thing that might be of interest is setting the architecture tag
(netwerk/protocol/http/OptimizedFor.h) to "386", but don't commit this -- just
do it for the local build. The update checker will still reference the PPC
build IDs currently but that's fine for now and I can actually change this on
the server. See also IntelBuild for some notes I wrote up.
Original comment by classi...@floodgap.com
on 24 Jan 2013 at 2:25
The Intel notes look good. That's right about what I had in mind as far as
release availability, etc.
I used the __PR atomic set function but called it TenFourFoxAtomicSet, and
undid the relevant changes in pratom.h. There is some speedup with Sunspider,
going down about 20ms. Not bad.
I confess, though, I don't quite understand the implications of the alignment
with the ASM code. I used .align 4, but I have no idea if that's ideal for
performance, caching, etc. I looked at the OSAtomic source in libkern and saw
that none of the i386 functions specified alignment. I think 4 is the default?
Do you build the final executable with 'make package,' or is there another way
to get a .app that stands on its own?
Original comment by lei...@gmail.com
on 24 Jan 2013 at 8:47
I don't know what the optimum alignment is for i386. Mozilla uses .align 4,
though, so that's probably "right." The alignment is probably for cache line
purposes.
The apps are simply generated with cp -RL obj-ff-dbg/dist/TenFourFox.app
/dest/in/a/tion/TenFourFoxIntel.app for ESR 10 and 17. 19+ has its own special
packager. Then just zip it up for shipping using the Finder's Archive command.
I don't distribute in .dmg right now.
Original comment by classi...@floodgap.com
on 25 Jan 2013 at 12:12
(When you're satisfied with it, feel free to attach that patch for your NSPR
atomics to issue 205.)
Original comment by classi...@floodgap.com
on 25 Jan 2013 at 12:13
Holding open until I've merged the patches into 17.0.3.
Original comment by classi...@floodgap.com
on 29 Jan 2013 at 9:29
Merging. One last note: we should find out why MOZILLA_MAY_SUPPORT_SSE2 in
nsUTF8Utils.h isn't getting set. I'll take your fixes as is for the moment,
just as a future maintainability question.
Original comment by classi...@floodgap.com
on 16 Feb 2013 at 12:51
Marking done since the PPC builds work fine, so I think this integrated
correctly.
Original comment by classi...@floodgap.com
on 16 Feb 2013 at 5:35
Original issue reported on code.google.com by
lei...@gmail.com
on 23 Jan 2013 at 2:44Attachments: