Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 315420

Summary:	Optionally use alternate file reading interface for Elf file access?
Product:	[Tools] CDT	Reporter:	Ed Swartz <ed.swartz>
Component:	cdt-debug	Assignee:	cdt-debug-inbox <cdt-debug-inbox>
Status:	NEW ---	QA Contact:	Jonah Graham <jonah>
Severity:	critical
Priority:	P3	CC:	john.cortell, pawel.1.piech
Version:	7.0	Keywords:	performance
Target Milestone:	---
Hardware:	PC
OS:	Windows XP
Whiteboard:

Description Ed Swartz

2010-06-02 12:37:09 EDT

The org.eclipse.cdt.utils.Elf class uses RandomAccessFile and long/short/byte wise access to iterate the contents.  

This is extremely slow over networks.  For EDC clients, which rely on this class, this means it can take a minute or more to start a debug session. 

For EDC we are already using a well-tested buffering file reader with a similar interface and would like to use this, without doing something nasty like copying the entire Elf class.

Since ERandomAccessFile already extends RandomAccessFile, and it marked "noextend", could I propose a late 7.0 modification like this, to minimize API changes while allowing us to work around it:

1) Adding an interface IRandomAccessFile, marked @noextend and @noimplement, with the same methods in ERandomAccessFile -- e.g., extending DataInput and adding the endian-aware support methods

2) Making ERandomAccessFile implement this new interface 

3) Changing Elf's constructor to accept IRandomAccessFile in addition to ERandomAccessFile, and change the 'efile' method to be a IRandomAccessFile

Then, for the use case in EDC where this is bothering me, I can use a different implementation of IRandomAccessFile and donate it to CDT core in the next release.

I think this will be API compatible, but it does add API.  Any feedback on this proposal for 7.0 final?  (We'll otherwise be stuck with this performance issue for 2 more weeks until 7.1.)

Comment 1 Ed Swartz

2010-06-02 17:15:12 EDT

Given the proximity to the 7.0 release (and the deafening silence ;), we'll just fork this class for EDC until the CDT 7.1 or 8.0 timeframe.

Comment 2 John Cortell

2010-06-02 17:37:54 EDT

(In reply to comment #0)
> 3) Changing Elf's constructor to accept IRandomAccessFile in addition to
> ERandomAccessFile, and change the 'efile' method to be a IRandomAccessFile

I'm a little confused. Elf doesn't accept an ERandomAccessFile; it creates one from the file path its given at construction. Are you talking about adding a new constructor that takes an IRandomAccessFile instead of a string?

Comment 3 Ed Swartz

2010-06-02 17:44:27 EDT

(In reply to comment #2)
> (In reply to comment #0)
> > 3) Changing Elf's constructor to accept IRandomAccessFile in addition to
> > ERandomAccessFile, and change the 'efile' method to be a IRandomAccessFile
> 
> I'm a little confused. Elf doesn't accept an ERandomAccessFile; it creates one
> from the file path its given at construction. Are you talking about adding a
> new constructor that takes an IRandomAccessFile instead of a string?

Yes, sorry, I want to add another constructor.

The API concern would be if someone actually did extend this class outside of the main CDT tree and depended on the type of 'efile'.

(Another reason I'm deflecting this to 8.0 is, I noticed that two versions of AR and maybe MachO may also depend on this, so it seems like it would be bad to fix this in only one client.)

Comment 4 John Cortell

2010-06-02 17:58:04 EDT

(In reply to comment #3)
> Yes, sorry, I want to add another constructor.
> 
> The API concern would be if someone actually did extend this class outside of
> the main CDT tree and depended on the type of 'efile'.
> 
> (Another reason I'm deflecting this to 8.0 is, I noticed that two versions of
> AR and maybe MachO may also depend on this, so it seems like it would be bad to
> fix this in only one client.)

If the new interface had all the public and protected methods in ERandomAccessFile, RandomAccessFile, DataOutput, DataInput and Closeable, I think that would be OK. I don't see any public fields in any of those types. Oh, but what if a derivative passes efile to another method? They would for sure be broken. Yeah. Post 7.0 sounds best to me.

Comment 5 Ed Swartz

2010-06-02 18:12:45 EDT

(In reply to comment #4)
> 
> If the new interface had all the public and protected methods in
> ERandomAccessFile, RandomAccessFile, DataOutput, DataInput and Closeable, I
> think that would be OK. 

Well, I wouldn't go as far as doing all the writing methods from those ancestors -- all the Elf/AR clients are doing is reading the content.

My work in progress is actually calling this IRandomReadAccessFile.

> I don't see any public fields in any of those types.
> Oh, but what if a derivative passes efile to another method? They would for
> sure be broken. Yeah. Post 7.0 sounds best to me.

Ok. :)

Comment 6 Jonah Graham

2019-12-30 18:54:31 EST

This bug was assigned and targeted at a now released milestone (or Future or Next that isn't being used by CDT). As that milestone has now passed, the milestone field has been cleared. If this bug has been fixed, please set the milestone to the version it was fixed in and mark the bug as resolved.