Closed Bug 180154 Opened 22 years ago Closed 14 years ago

freeze nsISeekableStream

Categories

(Core :: XPCOM, defect, P3)

Product:
Core
Component:
XPCOM
Type:
defect

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
Future

People

(Reporter: darin.moz, Unassigned)

References

(
URL
)

Details

(Keywords: arch, embed)

Attachments

(3 files, 3 obsolete files)

nsISeekableStream.idl draft
3.33 KB, text/plain
Details
handle local folders > 2GB and < 4GB (checked in)
16.57 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
patch addressing Darin's last comments - non-mail diffs only (checked in)
30.04 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review 
embedders, consumers of nsIUploadChannel probably need a way to rewind streams.
 personally, i'm happy with the interface as is.  we'd need to beef it out with
some documentation.  specifically, it should be made clear that |setEOF| may be
unimplemented or otherwise generate an error (e.g., can only truncate a writable
stream).
    /* 
     * Sets the stream pointer to the size of the stream plus the value 
     * of the offset parameter. 
     */

Should that say "minus"?  I'm not sure why one would want to position the
pointer past the end....
Er, nevermind.  That's obviously supposed to take a negative offset.... still a
little confusing, but...
Should this interface use some kind of typedef for the file offsets rather than
fixing it at PRUint32. Just thinking ahead when we have gigabit internet
throughput and people are browsing multi-gigabyte HTML pages ;-) Realistically
though I've started seeing downloads that are greater than 1gig. So this may not
be that far off.
    void seek(in long whence, in long offset);

    PRUint32 tell();

Should the return value type of tell maybe be the same as the type of the offset
argument of seek?
Oh yeah, and fwiw I agree with dbradley...

Can we make this simply use a 64 bit type? NSPR has functions for such file
accesses, at least Linux supports such files, and files are getting larger and
larger, so I don't think we should freeze the 32 bits into an interface.

Also note that nsIFile uses:
212     attribute PRInt64 fileSize;
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsIFile.idl#212
(I wonder why it's signed...)
the reset of necko is 32 bit.  we don't directly support long long
buffers/offsets.  

David - the whence bits are a direct mapping of NSPR constants.  NSPR hasn't
added any in years, why would we?

I will fix up the PRInt32 --> long thing.

>David - the whence bits are a direct mapping of NSPR constants.  NSPR hasn't
>added any in years, why would we?

Not sure what you mean here. I was only talking about the size of the "offset"
and how 2gigs seemed like a rather small limit for this interface. 


david, ignore that part of the comment.  Basically my point was that the 32bit
offset is consistent with the rest of Necko APIs.  
see:

nsIStreamListener::OnDataAvailable(..., PRUint32 offset, PRUint32 count)
nsIInputStream::Available(PRUint32 *result)
nsIChannel::GetContentLength(PRInt32 *result)

these are all frozen interfaces.  we'd also need to rev these interfaces if we
cared about supporting >2G files.  should support for >2G files really be a
requirement right now?
IMO we should serious consider support of downloading files >2gigs, but I don't
know if these interfaces are involved or not. While even with broadband this can
take a while, I'm sure we'll see people swaping large files, game demos, mpegs, etc.
this would be a pretty big overhaul of necko.  I just created the tracking bug
184452.





Damn, what the hell was I thinking.  I apologize for being a dumbass – too
preoccupied with the rumor mill.
i'm ok with freezing this interface.  i do have a couple nits though...

 - NS_SEEK_SET ... it's unfortunate that we have to have this NS_ prefix.  any 
   clever ideas of how we could eliminate that?

 - |const PRInt32| should probably be |const long| to match the type of |whence|.

otherwise, i think this interface is a good one to freeze.  (i'm also a little
concerned about the fact that some of our implementations only implement Tell. 
this is one of those interfaces where a reference to the interface is not really
an indication of what functionality is really available.  maybe that's an
implementation bug.)
> any clever ideas of how we could eliminate that?

Why not just rename the constants?  We'll have to change some implementations,
but....
but to what?  SEEK_SET is #define'd by most operating systems.  suppose the OS
values are not compatible with the NSPR values, which these are based on?  so,
any suggestions?
Ah.  Didn't realize we had a name collision...  No bright ideas offhand
(GK_SEEK_SET is just as bad as NS_SEEK_SET).


We could just use the OS values if they exist, couldn't we? People shouldn't be
hardcoding constants, and since c+ modules would have to be recompiled on a new
os, theres no compat issue, is there? Although that may be expsing more of the
internals than we want to.
bbaetz:

you're trusting that the NSPR constants map directly to the OS constants.  i
don't know if that is a safe thing to assume.  cc'ing wtc for advice.

we could also just #undef SEEK_SET, etc. above the interface decl.  that would
only cause minor pain for those who need to #include both our header file and
the stdc header file(s).  this might be worth the hassle.
Well, thats what I meant about perhaps exposing the interface too much. This is
just a wrapper arround fseek though, and so the constants' meanings are fairly
standard. Then you'd only have an issue where an os defined SEEK_SET and made it
mean something else

But yeah, it probably isn't a good idea to reuse those constants. We could
#undef them first, I suppose, but then people couldn't use themin teh same file,
and so on. |static const int|, perhaps?
You can't assume that the NSPR constants map directly to
the OS constants.
Why the setEOF()? I mean the rest of the interface (and the name) implies
read-only access to the stream.. would there be any difference between calling
close() on the actual stream? It wouldn't surprise me if there was.. I just find
it odd to see it in this interface, it seems like it belongs somewhere else.
alec: SetEOF means truncate at the current offset.  This follows the windows
convention of seek then set eof to truncate at a location in the stream.  SetEOF
is not at all equivalent to Close.
I think the issue is, that nsISeekableStream denotes read only access to the
file. So finding a method that would actually modify the file seems a little out
of place. I could have sworn I saw this same discussion somewhere, but wasn't
able to locate it.
well, i guess we can move it to a different interface.  maybe one that inherits
from nsISeekableStream?  the issue is that we don't want to make it so awkward
to seek and truncate.
however, i just realized (and confirmed via LXR) that after my changes for bug
176919, nsISeekableStream::setEOF is never called.  (NOTE: nsFileTransport.cpp
is the only caller, but that code is dead and needs to be CVS removed.)

so, perhaps the solution is simple.  perhaps we might want to just eliminate
SetEOF.  now, it is of course a useful function in many cases, but if we aren't
using it now, then maybe there is not good reason to freeze an interface which
provides it.
This is the start of one discussion on setEOF
http://bugzilla.mozilla.org/show_bug.cgi?id=86474#c32 I don't think it's the one
I was thinking of, though.
darin agreed to own xpcom.io.
Assignee: dougt → darin
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.4beta
seek and truncate seems fine, I just know there are lots of consumers and
implementors of input streams where you don't want to be modifying the
underlying stream..

i.e. whereas seeking and reading are commonly paired and require two distinct
interfaces, seeking and truncating is (I would assume) used much less often so I
see no reason to make a seeking interface overly complex to make that particular
pairing..
Target Milestone: mozilla1.4beta → mozilla1.5alpha
Target Milestone: mozilla1.5alpha → Future
mailnews needs at least the ability to use the full 32 bits of file size, so we
can have files up to 4GB, instead of being limited to 2GB. (Seek taking a signed
instead of an unsigned is the issue there). Having 64 bit files in mailnews
would be a whole new chunk of work...one possibility is having a new SEEK type
which treats the offset as an unsigned int, instead of treating negative numbers
as a reverse seek, something like NS_SEEK_CUR_UNSIGNED. Obviously, this is ugly
and non-standard, but on the plus side, it might allow us to go from 2GB->4GB
pretty easily.
why not use PRInt64 for seek types? mailnews wouldn't need to switch entirely to
64 bit functions, it could just convert an unsigned long to a PRInt64 when it
wants to call Seek..
well, hows about this. We have a seekForward() and seekBackwards() (or seek()
and rewind()), each of which take a long long - then we can still support 4G. 

It also syntacically defines defines two semantically different actions. I doubt
there is ever a time that someone is going to want to seek an arbitrary amount
forward or backward without knowing if they want to go forward or backward in
the first place. 

Furthermore, we could make a nsIRewindableStream that contains the rewind()
call, so that streams could indicate this capability to rewind via QI response.
converting mailnews is not a big issue, since we're not using nsISeekableStream
much, if at all. We'd like to get rid of nsFileSpec and use nsISeekableStream
and nsFileStream, but we'd like to get around the 2GB limit. I was more worried
about the uses of nsISeekableStream in the rest of the tree and converting those
to a 64 bit interface.
Verbose naming and a "rewindable" interface just makes the API story ulgier. 

I vote for either adding a nsISekableStream64 or fixing up the nsISeekableStream
to take 64 bit value. I prefer the former.
A new 64 bit interface would seem the easiest way to introduce it to the code
base without affecting the existing code. If we're unsure of the impact of 64
bit ints in the existing interfac,e going that route would be most prudent. If
"we're" comfortable with the impact it would be great to convert the existing
interface, and open up the ability to handle large files/streams throughout the app.
Darin and I were talking about this a bit - it seems like the least intrusive
fix in terms of the rest of the code would be to add seek64 and tell64 methods
to nsISeekableStream, since it's not a frozen interface, and Darin wants to
remove setEOF anyway. Adding a new nsISeekableStream64 interface would add a bit
of vtable bloat and make the client code QI for the new interface.

Converting nsISeekableStream to 64 bit methods is probably not prohibitively
difficult. From lxr, it looks like there are around 15-20 different files that
use nsISeekableStream::seek, and many of them are seeking to the beginning of
the stream anyway.

I can try either of these. I just need some sort of consensus and buyin...
if there are that few callers, I say just update the interface and them - I
would hope the pattern would be pretty straight forward.. we've done much more
extensive changes in the past :)
I'd probably be more concerned about tell than seek, since it's likely to have
more of ripple effect. I only see a handle full of those, but they're doing
things like storing the result out into data members, which would have to be
changed, and anything that touches those. That still may not be a big deal given
the small number of references to tell I saw on lxr, but I didn't walk more than
the top level So there could be one instance that fans out to many things that
in turn fan out into many more things.
Attached patch crude patch that builds and runs (obsolete) — — Splinter Review 
This patch builds and runs with 64 bit seek and tell - it's crude, and for the
most part, just treats the 64 bit result of tell as a 32 bit value. It also
doesn't use our 64 bit int macros, which Darin says might still be used in
embedding environments. I'm going to look at nsInt64 and see if I should be
using that...but, this patch does allow mail files to be > 2GB. I think most of
the users of nsISeekableStream will still end up just using the 32 bits, like
fastLoad, which isn't going to need files > 2GB, and is storing offsets in the
data file as 32 bit numbers anyway. And the StringStream class probably doesn't
need to worry about strings > 2GB...Anyway, this is the work in progress.
Does anyone else have input about the 64bit math? Does nsInt64 do the right
thing on platforms that don't support 64 bit ints directly? I'd like to see what
progress I can make on driving this forward.
Blocks: 207400
> Does nsInt64 do the right thing on platforms that don't support 64 bit ints
directly?

nsInt64 should work correctly on all platforms, even those that do not natively
support 64-bit integer types.
this patch addresses Darin's comment about wrapping 64 bit operations with
nsInt64. I've separated out the mailnews diffs into a different patch.
Attachment #139776 - Attachment is obsolete: true
Mailnews part of patch. I had to stop using nsMsgKeySet for the new msg set
because it doesn't like id's > 0x7fffffff. nsMsgKeyArray will take up more
memory but it does simplify the code a bit. In normal situations, there won't
be a lot of new messages, so I don't think it's a big deal.
Attachment #143825 - Flags: review?(darin)
Comment on attachment 143825 [details] [diff] [review]
non-mail part of patch, using nsInt64

>Index: xpcom/io/nsISeekableStream.idl

>+    void seek(in long  whence, in long long offset);
>+    unsigned long long tell();

hmm... i think it might make more sense for tell() to return a signed
long long instead.  it seems to me that you should be able to seek to
any position indicated by tell.  also, the lseek system call uses 
signed integers for both the current stream position and the new
stream position.  PR_Seek64 behaves just like lseek.

if you make tell return signed long long then you avoid the casting
from PRUint64 to PRInt64.  that casting worried me anyways since it
means we really don't support the full range of PRUint64.

i think this change might simplify the patch considerably.

nit: i noticed some off-by-one character indentation problems,
especially in member variable declarations.
Attachment #143825 - Flags: review?(darin) → review-
sure, I can do that - I was just following the existing behaviour of making tell
return an unsigned value. I wasn't happy about the casts either...
(In reply to comment #46)
> sure, I can do that - I was just following the existing behaviour of making tell
> return an unsigned value. I wasn't happy about the casts either...

yeah, i figured that was the case.  i hadn't given this much thought until
looking at your patch and seeing what a pain all those casts are.
this does remove all the casts - I can't say it makes the patch much simpler,
but there aren't any more casts to PRInt64. I've also made nsStorageStream just
do 32 bit streams - if we want to make that support larger streams, we can do
that orthogonally...
Attachment #143825 - Attachment is obsolete: true
Comment on attachment 144054 [details] [diff] [review]
patch addressing Darin's comments - non-mail diffs only

>Index: netwerk/base/src/nsInputStreamPump.cpp

>+                    nsInt64 offsetBefore64 = offsetBefore;
>+                    nsInt64 offsetAfter64 = offsetAfter;
>+                    if (offsetAfter64 > offsetBefore64)
>+                        mStreamOffset += (PRInt32) (offsetAfter64 - offsetBefore64);

nit: maybe add an assertion that the difference fits in a PRInt32?  not that
that isn't going to always be the case ... ;-)


>Index: xpcom/io/nsFastLoadFile.cpp

>+    NS_ASSERTION(fileSize < 0xFFFFFFFF, "fileSize must fit in 32 bits");
>+    // todo - assert fileSize fits in 32 bits;

maybe this "todo" is bogus now?  also, do you need to use LL_ macros of
nsInt64 inside this NS_ASSERTION?


>+    PRInt64 footerOffset;
>+    rv = seekable->Tell(&footerOffset);
>+
>+    mHeader.mFooterOffset = (PRUint32) footerOffset;

this cast needs to use LL_ macros?


>+    PRInt64 fileSize;
>+    rv = seekable->Tell(&fileSize);
>+    mHeader.mFileSize = (PRUint32) fileSize;

same here.


>Index: xpcom/io/nsStringStream.cpp

>+    NS_ASSERTION(offset < 0xFFFFFFFF, "string streams only support 32 bit offsets");

use LL_ macros to compare or use nsInt64?


>Index: xpcom/obsolete/nsFileStream.h

>+                                          PRInt64 result = -1;

use LL_ macros?


>Index: xpcom/obsolete/nsIFileStream.cpp

>+        case NS_SEEK_SET: ; break;

semicolon before break should not be needed.


>+    nsInt64 zero = LL_Zero();

maybe just do:

  nsInt64 zero = 0;


r=darin with these nits picked.
Attachment #144054 - Flags: review+
this addresses Darin's comments, assuming I haven't missed anything :-)
Attachment #144054 - Attachment is obsolete: true
Attachment #144227 - Flags: superreview?(mscott)
Comment on attachment 143826 [details] [diff] [review]
handle local folders > 2GB and < 4GB (checked in)

corresponding mailnews changes
Attachment #143826 - Flags: superreview?(mscott)
Minor nit if you think of it before you check it in, the minus1 declarations
could benefit from adding "const". There were a few other places
(fileSize64,maxUint32, etc.)  where, I assume for conversion reasons, you assign
and never change the value, those could use const as well. Most smart compilers
would probably figure it out.
Attachment #144227 - Flags: superreview?(mscott) → superreview+
Attachment #143826 - Flags: superreview?(mscott) → superreview+
One more thing that needs to change:  The IID of nsISeekableStream must be
changed.  See bug 239038, and a comment I posted to super-reviewers@mozilla.org
I went ahead and bumped the interface IID:

Checking in nsISeekableStream.idl;
/cvsroot/mozilla/xpcom/io/nsISeekableStream.idl,v  <--  nsISeekableStream.idl
new revision: 3.9; previous revision: 3.8
done

It appears that the latest patch landed earlier today.
Blocks: 242583
Blocks: 242591
Attachment #143826 - Attachment description: handle local folders > 2GB and < 4GB → handle local folders > 2GB and < 4GB (checked in)
Attachment #144227 - Attachment description: patch addressing Darin's last comments - non-mail diffs only → patch addressing Darin's last comments - non-mail diffs only (checked in)
Blocks: 268520
QA Contact: scc → xpcom
Assignee: darin → nobody
Status: ASSIGNED → NEW
We no longer freeze interfaces.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: