Closed Bug 1297331 Opened 3 years ago Closed 2 years ago

Remove declarations of some unused NSPR functions from jsnspr

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: Ms2ger, Unassigned)

Details

Attachments

(1 file)

No description provided.
I think we can also remove the JS_CallOnce API, so we can get rid of PR_CallOnce* too. It's a pretty weird API for something that has little to do with JS (embedders have other ways to do it), and it's not used in Firefox.
I agree, but that involves JSAPI changes, so I'm calling scope creep :)
Comment on attachment 8783866 [details] [diff] [review]
Remove declarations of some unused NSPR functions from jsnspr

Review of attachment 8783866 [details] [diff] [review]:
-----------------------------------------------------------------

I'm pretty sure Luke is actually in the process of landing the code that uses these.
Attachment #8783866 - Flags: review?(nfitzgerald) → review?(luke)
(In reply to Jan de Mooij [:jandem] from comment #2)
> I think we can also remove the JS_CallOnce API, so we can get rid of
> PR_CallOnce* too. It's a pretty weird API for something that has little to
> do with JS (embedders have other ways to do it), and it's not used in
> Firefox.

Filed bug 1297404.
Comment on attachment 8783866 [details] [diff] [review]
Remove declarations of some unused NSPR functions from jsnspr

Review of attachment 8783866 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, sorry to rain on the whole "kill NSPR" train; bug 1276029 contains the patch that uses these.  If anyone wants to add platform-independent memory-mapping functions to MFBT that can consume PRFileDesc's (what we ultimately start with in Gecko), I'd be happy to use them instead!
Attachment #8783866 - Flags: review?(luke) → review-
> Yes, sorry to rain on the whole "kill NSPR" train; bug 1276029 contains the patch that uses these.  If anyone wants to add platform-independent memory-mapping functions to MFBT that can consume PRFileDesc's (what we ultimately start with in Gecko), I'd be happy to use them instead!

I very disappointed to see the ambition to get rid of NSPR die with this one penstroke.

Just use native posix & windows memory-mapping functions in WasmModule.cpp; it'll actually make it more readable.
(I will pass the opportunity to ridicule the implementation of Truncate() in the proposed patch.)

All the functions you need:
posix:                   
fstat() -> get the size
ftruncate() -> change the file size
mmap() -> map a file
munmap() -> unmap a file

windows: 
_get_osfhandle() -> fd to HANDLE
GetFileInformationByHandle() -> get the size
SetFileInformationByHandle() -> change the file size
CreateFileMapping() + MapViewOfFile() -> map a file
UnmapViewOfFile() + CloseHandle() -> unmap a file
(In reply to Bert Belder from comment #7)
> posix:                   
> mmap() -> map a file
> munmap() -> unmap a file
> 
> windows: 
> CreateFileMapping() + MapViewOfFile() -> map a file
> UnmapViewOfFile() + CloseHandle() -> unmap a file

I have no idea how well this maps to the desired API here, but we actually have js::gc::AllocateMappedContent() and js::gc::DeallocateMappedContent() for this part [1].

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/gc/Memory.h#39
If these are the only functions holding back some major simplifications, then, sure, go for it.  We can add #ifdefs and/or reuse AllocateMappedContent in the serialization patches.
Attachment #8783866 - Flags: review- → review+
ctypes still relies on NSPRs dynamic library loading. That's the last thing other than this bug, but isn't exactly tiny...
Assignee: Ms2ger → nobody
Status: ASSIGNED → NEW
Fixed in bug 1004923.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.