1297331 - Remove declarations of some unused NSPR functions from jsnspr

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Remove declarations of some unused NSPR functions from jsnspr

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I agree, but that involves JSAPI changes, so I'm calling scope creep :)

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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 6

[Luke Wagner [:luke]]

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!

Comment 7

[Bert Belder]

> 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

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

(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

Comment 9

[Luke Wagner [:luke]]

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.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

ctypes still relies on NSPRs dynamic library loading. That's the last thing other than this bug, but isn't exactly tiny...

Comment 11

[Jason Orendorff [:jorendorff]]

Fixed in bug 1004923.