Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: fix MemoryUsage for positional and component objects #1044

Merged
merged 3 commits into from
Jan 13, 2017

Conversation

fingolfin
Copy link
Member

Fixes #897

@codecov-io
Copy link

codecov-io commented Dec 28, 2016

Current coverage is 50.46% (diff: 85.08%)

No coverage report found for master at 34e30fb.

Powered by Codecov. Last update 34e30fb...52f7449

@frankluebeck
Copy link
Member

This looks all good to me (except that in the "generic fallback method" the last else clause could be deleted).

# we were the first to be called, thus we have to do the cleanup
ClearObjectMarker( MEMUSAGECACHE );
fi;
# objs without subobjs
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frankluebeck You mean this clause?

I don't mind deleting it, but I had a reason why I left it in: Namely as a reminder that there can be other object types... Though the comment is indeed misleading! For such objects can actually contain subobjects, we just don't have a way to access them! For example, in SingularInterface and NormalizInterface we do this. Perhaps an InforWarning would be appropriate? But then we'd have to take care to only show it for external objects

So, should I remove this? Or instead change it to something like this:

elif TNUM_OBJ_INT(o) >= FIRST_EXTERNAL_TNUM then
  Info( InfoWarning, 1, "MemoryUsage encountered an external object, results may be too low" );

Note that I do not suggest to use TryNextMethod because this is already the fallback method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the else can vanish. But the proposed warning for new objects with external TNUM sounds like a good idea. For such objects a method for MemoryUsage should be installed.

(Actually, for objects like in the SingularInterface it is not obvious what MemoryUsage should return. Just the space occupied within GASMAN or also the external space, where the latter may not be easy to determine.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not so clear -- so a warning seems useful, so that one at least knows that there is a hidden iceberg... :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the warning, but I had to move MemoryUsage to a new file, as InfoWarning is not available in object.gi. But I actually think this is better anyway.

@fingolfin
Copy link
Member Author

I added a warning that is shown when MemoryUsage is invoked for an external object for which no custom method has been installed. To do that, I had to move MemoryUsage from object.{gi,gd} to new files, as InfoWarning is not available in object.gi. But I actually think this is better anyway.

Also rebased this on master, to resolve a conflict with that.

@frankluebeck Does that seem OK to you now?

Copy link
Contributor

@ChrisJefferson ChrisJefferson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this.

@fingolfin
Copy link
Member Author

So, merge this? Rebase it? Should I do it myself?

@ChrisJefferson
Copy link
Contributor

Just taking a brief break from merging to allow all tests to pass in master. Then I'll rebase this in too.

@ChrisJefferson ChrisJefferson merged commit 1cb0208 into gap-system:master Jan 13, 2017
@fingolfin fingolfin deleted the mh/memoryusage branch January 13, 2017 22:48
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Jan 20, 2018
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants