bug-make
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [bug #33138] .PARLLELSYNC enhancement with patch


From: Frank Heckenbach
Subject: Re: [bug #33138] .PARLLELSYNC enhancement with patch
Date: Thu, 25 Apr 2013 02:16:33 +0200

Paul Smith wrote:

> On Wed, 2013-04-24 at 20:46 +0200, Frank Heckenbach wrote:
> > That's true about SEEK_CUR which was there originally. I actually
> > changed it to SEEK_END, which does move the position to the end.
> 
> Oh right.  My head cold is keeping me foggy.  What was the reason to
> change to SEEK_END, again?

lseek (SEEK_END, 0) seeks to the end and then returns the current
position, i.e. the file size.

lseek (SEEK_CUR, 0) stays at the current position and returns it. So
if the fd (in the parent) remains at the start while the child wrote
something, it would return 0, whereas the former wouldn't.

AFAIK, lseek (SEEK_END, 0) is one of two canonical ways (besides
fstat) to get the size of a file.

Eli Zaretskii wrote:

> > > I just mentioned some caveats that people might find surprising,
> > > that's all.  Perhaps those caveats should be mentioned in the
> > > documentation.  No other intentions.
> > 
> > OK, we can add a few sentences in the docs. Have you already written
> > something (just to avoid further dupliation of work)?
> 
> No.  Go ahead, if you have time.

Attached.

> > > As such, using just 'int'
> > > for sync_handle is not wide enough, certainly not in 64-bit builds.
> > > Is it OK to use intptr_t instead?  Doing this cleanly might require to
> > > have a macro (I called it FD_FROM_HANDLE) to extract a file descriptor
> > > that is passed to a Posix fcntl; on Windows that macro will be a
> > > no-op, and on Posix platforms it's a simple cast.
> > > 
> > > Is this OK?
> > 
> > I can't follow you here. On POSIX, we don't need to pass a fd
> > because it's always stdout/stderr. Or do mean something else?
> 
> I meant the file descriptor passed to fcntl to put a lock on it.  The
> value of that descriptor is stored in sync_handle, whose type is
> 'int'.

So you want to overload sync_handle to contain an fd on POSIX and a
pointer on Windows? I'm not sure I'd like this. Since it's used in
separate code branches (ifdef'd or perhaps in different source files
in compat) anyway, why not define separate variables of the
appropriate types (preferably with different names, to reduce
confusion)?

> > On Windows, you said fstat was very expensive, didn't you? Or is
> > lseek even worse?
> 
> I think anything that potentially moves the file pointer can be
> sometimes expensive and is best avoided.  (On Windows, I'd use
> GetFileInformationByHandle.)

OK, if that's so, do that. But I don't think that's true on POSIX.

> > Nothing is actually read by lseek (and even if it were, it would
> > only need to look at the first and last part of the file, not read
> > all the content, if that was the worry).
> 
> Are you sure?  How can lseek "jump" to the last byte of the file, if
> the file is not contiguous on disk, except by reading some of it?

lseek doesn't need to read any data. It just sets the current offset
of the FD to the given position, so the next read (which in this
case never happens before seeking to the beginning) knows where to
read. Even in the case of SEEK_END, all it has to do is add the
given offset (here: 0) to the current file size.

Instead of testing, I just looked at the implementation (Linux
3.2.2). The following is really the whole relevant code. As you see,
nothing's read from the disk, it only handles in-memory data. (Also
the file size is in memory for open files; even it were not, it
would be a constant-time access to the inode and wouldn't need to
touch any data blocks.)

loff_t
generic_file_llseek_size(struct file *file, loff_t offset, int origin,
                loff_t maxsize)
{
        struct inode *inode = file->f_mapping->host;

        switch (origin) {
        case SEEK_END:
                offset += i_size_read(inode);
                break;
        // skipped other cases
        }

        return lseek_execute(file, inode, offset, maxsize);
}

static loff_t lseek_execute(struct file *file, struct inode *inode,
                loff_t offset, loff_t maxsize)
{
        if (offset < 0 && !unsigned_offsets(file))
                return -EINVAL;
        if (offset > maxsize)
                return -EINVAL;

        if (offset != file->f_pos) {
                file->f_pos = offset;
                file->f_version = 0;
        }
        return offset;
}

static inline int unsigned_offsets(struct file *file)
{
        return file->f_mode & FMODE_UNSIGNED_OFFSET;
}

static inline loff_t i_size_read(const struct inode *inode)
{
        // skipped code to deal with preemption and synchronized access in SMP
        return inode->i_size;
}
--- doc/make.texi.orig  2013-04-24 16:30:05.000000000 +0200
+++ doc/make.texi       2013-04-25 02:08:58.000000000 +0200
@@ -8639,6 +8639,17 @@
 complex parallel build in the background and checking its output
 afterwards.
 
+Note that with this option, each job's output is redirected to a
+temporary file first. Some commands can behave differently depending
+on the type of their standard output or standard error. E.g.,
address@hidden --color=tty} might display a colored directory listing when
+standard output is connected to a terminal. When using this option,
+colors would be disabled because the output of the command goes to a
+file. Note, however, that it is generally best to avoid such
+commands in makefiles, independent of this option, since the
+different behavior would also be triggered when users redirect the
+whole output of @code{make}, e.g. to a log file.
+
 @item -q
 @cindex @code{-q}
 @itemx --question

reply via email to

[Prev in Thread] Current Thread [Next in Thread]