3.55 KB, text/plain
4.34 KB, patch
|Details | Diff | Splinter Review|
281.77 KB, patch
|Details | Diff | Splinter Review|
31.37 KB, patch
Nelson Bolyard (seldom reads bugmail): review-
|Details | Diff | Splinter Review|
The BeOS code for NSPR is a bit messy right now. This bug is for cleaning up the NSPR-port for BeOS. I'll put up some work in progress code. This will ease up when improving the code for Haiku, http://www.haiku-os.org , or Zeta http://www.yellowtab.com
Created attachment 189161 [details] [diff] [review] Work in progress This is a just an early, not very clean, not complete, changeset to show what can be done. Note that I added some code to the atomic test just to measure performance, and that I added asm instructions for atomic ops on x86. Which has no measurable differance compared to BeOS builtin atomic ops, but the atomic set operation is faster, also I think the set operation should behave better with threads. Comments?
Spotted an error in btlocks.c: _MD_ATOMIC_DECREMENT( &lock->benaphoreCount, -1 ) should be _MD_ATOMIC_DECREMENT( &lock->benaphoreCount )
Uh, and the atomic.c is swamped with bugs.
Created attachment 190151 [details] [diff] [review] Very experimental Ok, here is one that works for Firefox under BONE (I know I've broken netserver) It may not work for Mozilla as NSPR might not be fully implemented. File and socket operations are no longer based on prfile, prdir and prsocket, which is the way pthreads works. I'm actually posting this with that nspr-implementation. (Dunno if I managed to get cvs diff to include added/removed files) Comments?
Created attachment 222379 [details] [diff] [review] NSPR-patch, almost done The socketpair code should probably be replaced, also need to test on netserver and I need to check that I implement all we need in primpl.h.
Created attachment 222380 [details] nsprpub/pr/src/bthreads/btfile.c
Created attachment 222381 [details] nsprpub/pr/src/bthreads/btio.c
tqh, I found the filed bug on this. Please let's keep this moving. I've been testing this code and notice better performance and no bad results.
Personally I think NSPR should be a bit reorganized (private.h at least), also with the code duplication in xpcom (lib loading, and others?) I would like to know what the future is for those components. Right now it seems NSPR is slowly fading away, so should we really put work into it?
NSPR may be slowly fading away from the trunk, but it is certainly a necessary component of the 2.0 branch. Since we're stuck in the Branch until Haiku is ready to support development, and since the work-in-progress yields measurable performance improvements, BeOS users would benefit from this clean up.
Yes, but I think that's easier to provide as a system NSPR library.
Created attachment 342776 [details] [diff] [review] NSPR for Haiku Pretty much how I want it. Still needs to be confirmed to work on R5, Zeta (and with gcc2). Also need to look over the patch so that no gcc4 build fixes has slipped in here. Most notable is the change of PRNetAddress to 32 bytes, which IS what at least BONE, Zeta and Haiku have. Not sure but this might have an effect for those with https problems.
I'll download, apply and test on R5, BONE and Zeta.
Created attachment 342821 [details] console output of build failure under Zeta I updated branch from CVS, deleted entire output directory, applied patch and did "clean build" nspr build fails with attached console output.
Stupid gcc2.95.3 :) Anyway it is both good and bad as that means I fixed the problem mmadia had when he tested a different version. New patch coming soon.
Same problem though, c in gcc2.95.3 only allow vars at beginning of code-blocks.
Created attachment 342925 [details] stack crawl of crash after start Just for fun, I moved the definitions to the beginning of the code block (right after "int32 cookie = 0" and it built OK; then had problem with bproc.c. Needed to put "#include signal.h" back in, because SIGKILL line 222 was not defined. From there, it built OK, however the resulting Firefox dies with a segment fault right after starting. See attachment for stack crawl.
Created attachment 342926 [details] [diff] [review] Patch 3 some reorder of prlink.c Check if this one builds. No fix for your stackcrawl yet.
Stackcrawl is probably in this call: http://mxr.mozilla.org/mozilla1.8.0/source/nsprpub/pr/src/io/prpolevt.c#453 So it seems USE_TCP_SOCKETPAIR is defined, but it shouldn't be. Not sure where that is set, but can you check your build-dir's mozilla-config.h if it's there??
Nvm, it seems to be defined in prpolevt.c.
I'm thinking we need a real BeOS/Haiku impl of that file. And I suspect this could be cause of many weird crashes.
I've wondered about prpolevt.c in the past, though it's been so long since I've tried to understand it that I've forgotten what had me looking there in the first place. I'll try your prlink.c reording now.
patch 3 fixes prlink.c but bproc.c still needs that additional #include.
(In reply to comment #22) > Nvm, it seems to be defined in prpolevt.c. Yep, it's there. Line 409 set this for all OS except Unix. I remember the context now; I believe we tried once before to add "&& !defined(XP_BEOS)" to this line; this forces BeOS to use the code using pipes. But there's concern that BeOS' pipes implementation is broken so that's not a viable alternative, either. We may need our own implementation, as you say.
I believe that in Haiku pipes are OK
I've tried pipes under Zeta - while it may in theory be broken, the only thing I notice is Firefox is slower. You can definitely feel the occasional wait for the file system. Turns out, it only creates two pipe files, regardless of the number of windows or tabs open, so I wonder if tqh's idea of a dedicated "prpolevt.c" for BeOS might be developed.
Created attachment 344151 [details] [diff] [review] PR_PollableEvent hack I tried to rewrite PR_PollableEvent, but I'm not fully understanding how all things work together. Attaching the code here (it's not all socketfd and some others in NSPR was skipped) so that if someone can suggest what I've missed/messed up. Otherwise I'll just do the good AF_UNIX socketpair for Haiku and leave the old platforms to dust.
(In reply to comment #29) > Created an attachment (id=344151) [details] > Is it correct to assume this patch is to be applied alongside patch3 just above?
Created attachment 345011 [details] console output PR_PollableEvent hack does not build correctly. Console output of failure is attached.
looks like PR_CreateSocketPollFd and PR_DestroySocketPollFd are implemented in prsocket.c and ptio.c. To my untrained eye, it looks like we don't build either of these. Do we need to implement these in btio.c?
Created attachment 345049 [details] revised btio.c with additional code lifted from prsocket.c Looks like I answered my own question. I lifted the additional implementations from prsocket.c,(along with static const PRIOMethods* PR_GetSocketPollFdMethods code at around line 1891. newly hacked nspr with PR_PollableEvent seems to build OK.
It builds OK, however the resulting Firefox will not run. It does not open its initial window. If a previous session closed unexpectedly, it will prompt to resume or start new, however it never moves past this window once a selection is made.
It's code for discussing, not running. I did write that not all code needed was even included.
Comment on attachment 345049 [details] revised btio.c with additional code lifted from prsocket.c This is not really usable for anything.
(In reply to comment #35) > It's code for discussing, not running. I did write that not all code needed was > even included. I understand; you asked for someone to suggest what was missed and I thought you'd like to know the one small thing I found.
I wanted more feedback from some NSPR-guru, what makes the wms-version tick it doesn't seem like those functions even block.
Created attachment 345972 [details] [diff] [review] Final version? Is there even a remote chance to get a beast like this (292k patch) reviewed and commited into Firefox2?
tgh, I think this patch has a chance of getting reviewed. If you wish to request review, you need to put an explicit review request on your patch, naming the person whose review you seek, and you should also add that person's email address to the bug's CC list. Otherwise, it is likely that that person will be unaware of this bug or your patch for it. Here are a few things I observe after a quick glance at the patch. 1. Use of c++ style comment delimiters in c source files. Don't use // to begin comments in any source files that will ever be run through any non-gcc compiler. Many non-gcc compilers don't recognize c++ style comments. Maybe all the files that are ONLY built for BEOS will only ever be compiled with gcc, in which case it's OK to use // in those files. But files that are used on other platforms, e.g. prtypes.h must avoid that style of comment. 2. Your patch makes changes that affect ALL platforms built with gcc. That will require a lot of testing, testing on all platforms, which will significantly slow the approval of your patch. For quicker approval, make only changes that affect only the BEOS platform. 3. I'll bet the code conditionally compiled by this ifdef doesn't get included or omitted on platforms the way you think it does. #ifdef defined(_PR_PTHREADS) || defined(_PR_BTHREADS)
Thx, Nelson. I'm aware of the review-procedure, was just worried about the end of life of Firefox 2. 1. Oops. 2. I suspect you are referring to headers like primpl.h? If that is the case I'm a bit hesitant to change that as I want to have the BeOS NSPR-code as clean as possible. Other than that I think it can probably be adjusted. 3. This is definatly an error. Oops. 4. I saw some stuff that needs cleaning up as well.
Created attachment 346520 [details] [diff] [review] Patches outside BeOS dirs. Patches that needs to be done outside of bthreads and md beos-specific files.
Created attachment 346521 [details] [diff] [review] BeOS specific patch Patches to bthreads and md beos-specific files.
Comment on attachment 346520 [details] [diff] [review] Patches outside BeOS dirs. r? These are the changes I believe affect other platforms. NSPR fixes: * Make BThreads more like PThreads code. * Fix PRNetAddr len, which has been to small before. * NSPR GCC4 support under BEOS * Prlink don't need the stub hack under Haiku, and some reordering to avoid extra work.
Comment on attachment 346520 [details] [diff] [review] Patches outside BeOS dirs. Missing file in patch.
Created attachment 347809 [details] [diff] [review] Patches outside BeOS dirs updated. Added missing nsprpub/pr/src/Makefile.in change.
Comment on attachment 347809 [details] [diff] [review] Patches outside BeOS dirs updated. r?
See comment #44 for what changes the patch outside BeOS-dirs are supposed to correct. Note, it does work without the BeOS-specific changes, but I was hoping for a simpler review on those were it's concluded that it only affects BeOS/Haiku and that the code is not that bad.
Note, it does NOT work...
> Note, it does NOT work... What does that mean? Are you asking me to review a patch that is known to be defective?
No, it was a correction to comment #48, but it seems I was too tired to get even the correction right. It means that the patch outside of beos-dirs is part 1 of 2 and both of them needs to pass the mustard for it to work.
Comment on attachment 347809 [details] [diff] [review] Patches outside BeOS dirs updated. My comments are primarily about file prio.h. 1. BEOS is the only platform on which the declarations of these structures have different sizes than all the others. On all other platforms where NSPR's PRNetAddr doesn't match the local OS's structures, NSPR has platform-dependent code that copies one to the other, making adjustments as necessary. BEOS appears to be the only platform that avoids that copy work by using a platform dependent definition of these structures. One of the reasons for that is binary compatibility. Programs compiled with NSPR header files one year are supposed to "just work" with NSPR shared libraries built years later. If the OS continues to provide backward binary compatibility, then NSPR wants to also. That means that the sizes of the public NSPR structures (such as PRNetAddr) is not supposed to change from version to version. Now, perhaps an argument can be made that there is no existing body of BEOS application software that has already been built with older NSPR headers, and expects to benefit from that binary compatibility. But I'd like to see that case be made EXPLICITLY rather than just assuming it or ignoring the binary compatibility issue. If/Since you want to change these structure declarations for BEOS, I'd prefer to see the change be to make BEOS use the same declaration that all other platforms use. I realize that this file was already conditionally compiled for BEOS before this patch was written, but I'd really like to see the BEOS code be made like all the other platforms, IF it is to be changed at all. 2. Minor stylistic issue. I'm sure the NSPR module owners would prefer to see #ifdef XP_BEOS <BEOS CODE> #else <other platforms> #endif rather than #ifndef XP_BEOS <other platform code> #else <BEOS code> #endif There are many reasons for this, one of which is that if we should ever want to add a SECOND special platform, we could simply insert #elif defined(SPECIAL_PLATFORM) <Special Platform Code> rather than inserting more ifs, elses and endifs. This says that the code for special cases tends to come before the default case. So I think the big question to be answered here is: Do binary compatibilty requirements prevent prio.h from being changed as this patch does? If not, then the next question is: In the interest of isolating NSPR- based applications from further changes, shouldn't BEOS just use the same declaration as all other platforms, and take up the structure conversion methodology, like other platforms do, for binary compatiblity going forward? Wan-Teh, what's your opinion on these subjects?
1. Not sure I have the energy to rewrite that as well. Do note that I'm currently the only active BeOS/Haiku dev, and I have a lot more to do. (Next on task is enabling gcc4 for Haiku and Haiku as a platform.) 2. Understood. Big question: Will it even change the binary combability on any other platform than BeOS / Haiku? While you see other platforms as stable and mature, BeOS still isn't. This is one of the big things to get BeOS there, so if you really want to have backwards compability on BeOS, I think it should start AFTER this patch. When it comes to applications using NSPR, we have a few built by BeOS Mozilla devs and some by others (Seamonkey, Thunderbird and so on) but they are all out of date or waiting for this patch. We don't have apps that depends on NSPR on it's own (no Virtualbox) nor have we ever provided NSPR as a lib of it's own yet.
I think it's fine to break binary compatibility on BeOS if the maintainer of the BeOS port doesn't care about it now. tgh, we just wanted to make sure you know that NSPR guarantees backward compatbility and we expect the BeOS port to be backward compatibile when it's stable enough. Do you know why the various sockaddr_xxx structures have larger padding on BeOS?
No idea. Haiku uses it to stay compatible with BeOS (BONE networking stack), but Axeld of Haiku said: "We actually have different sizes per family, too, it's just that we stayed compatible with BeOS with regards to sockaddr and sockaddr_in. For sockaddr_in6 and others, there is no need to stay compatible, though. We also have a sockaddr_storage which is 256 bytes in size." For the complete discussion see: http://dev.haiku-os.org/ticket/1898
Comment on attachment 347809 [details] [diff] [review] Patches outside BeOS dirs updated. I've decided to give this patch r-. The issues I previously raised in comment 52 are serious enough to warrant this, IMO. At some time in the past, someone allowed the structures in prio.h, which are supposed to be platform independent, to be made platform dependent for just one platform, BEOS. IMO, that was a mistake. One of NSPR's goals is to provide its own virtual platform definition that works on all supported OSes and is indendent of those OSes' own definitions. By allowing BEOS to have its own platform-dependent definition of these structures, that goal is not achieved. Ordinarily, our binary compatbility requirements are SO STRONG that they would prevent a change being made that changes the sizes of members of public structures, even when those changes are to fix a bug. But in this case, it has been decided that now is an acceptable time to break backward binary compatibility for BEOS (only). So, rather than change the public structure declarations from one platform-dependent definition for BEOS to yet another platform-dependent definition for BEOS, this is the right time to undo that mistake of the past, and have BEOS use the same structure declarations as the other platforms use.
Then I think we can close this bug altogether.
Nelson, I understand and appreciate your desire to "right a previous wrong" as expressed in comment 56. However, I do not think this is the best time to redress this issue. For the last 5 years, we have had only two part-time devs working on the project. Changes to the trunk code (move to Cairo, requirements for gcc 3.x, etc.) have caused us to stop trunk development and focus our efforts solely on the 1.8.1 branch in order to serve our user base. Once the Haiku project progresses and we have a new OS to allow the move to gcc4, we expect to resume trunk development including cairo, etc. I would propose we change to standard structures at this time. To this end, I ask you to please reconsider your review and allow us to land tqh's patch ON THE 1.8.1 BRANCH ONLY. The new code makes a strong contribution to the reliability of the code on BeOS and Haiku.