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

Added files for linking Tril #7589

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@Yuehan-Lin
Copy link
Contributor

Yuehan-Lin commented Oct 24, 2019

Based on issue #5836 , added linkers needed for Tril source files

Signed-off-by: Mark Stoodley mstoodle@ca.ibm.com
Signed-off-by: Yuehan-Lin Yuehan.Lin@ibm.com

@Yuehan-Lin Yuehan-Lin force-pushed the Yuehan-Lin:tril5 branch 10 times, most recently from ea4004f to eee31e7 Oct 25, 2019
@fjeremic fjeremic requested a review from mstoodle Oct 30, 2019
@Yuehan-Lin Yuehan-Lin force-pushed the Yuehan-Lin:tril5 branch 2 times, most recently from 1485b2b to 65babba Oct 31, 2019
@0xdaryl 0xdaryl added the comp:jit label Nov 26, 2019
@Yuehan-Lin Yuehan-Lin force-pushed the Yuehan-Lin:tril5 branch 2 times, most recently from 2bb7f0d to dfd1eec Dec 5, 2019
Co-authored-by: Mark Stoodley <mstoodle@ca.ibm.com>
Signed-off-by: Yuehan-Lin <Yuehan.Lin@ibm.com>
@Yuehan-Lin Yuehan-Lin force-pushed the Yuehan-Lin:tril5 branch from dfd1eec to 128ec36 Dec 5, 2019
@0xdaryl

This comment has been minimized.

Copy link
Contributor

0xdaryl commented Dec 6, 2019

@dsouzai : would you mind reviewing the memory segment changes please?

Copy link
Member

dsouzai left a comment

I think there's a fundamental problem with the approach. Because you're creating a constructor that only takes in a TR::RawAllocator, you're forced to allocate the backing provider (J9::J9SegmentProvider) in the constructor of TR::SystemSegmentProvider; this would be fine if TR::SystemSegmentProvider didn't also inherit from J9::SystemSegmentProvider.

SystemSegmentProvider(size_t segmentSize, TR::RawAllocator rawAllocator)
: J9::SystemSegmentProvider(segmentSize,
segmentSize,
0,

This comment has been minimized.

Copy link
@dsouzai

dsouzai Dec 6, 2019

Member

Why would you want the allocation limit to be set to 0?

@@ -60,8 +62,11 @@ class SystemSegmentProvider : public TR::SegmentAllocator
size_t _allocationLimit;
size_t _systemBytesAllocated;
size_t _regionBytesAllocated;

protected:
J9::J9SegmentProvider & _systemSegmentAllocator;

This comment has been minimized.

Copy link
@dsouzai

dsouzai Dec 6, 2019

Member

nit-picky, but it's probably best to just move this to above the private keyword (and do the same in the .cpp file so that the initializer list runs in the same order as the definition).

{
if (_mySegmentAllocator)
{
static_cast<J9::SegmentAllocator *>(&_systemSegmentAllocator)->~SegmentAllocator();

This comment has been minimized.

Copy link
@dsouzai

dsouzai Dec 6, 2019

Member

This doesn't look right. your TR::SystemSegmentProvider inherits from J9::SystemSegmentProvider; in the destructor of J9::SystemSegmentProvider:

J9::SystemSegmentProvider::~SystemSegmentProvider() throw()
   {
   while (!_systemSegments.empty())
      {
      J9MemorySegment &topSegment = _systemSegments.back().get();
      _systemSegments.pop_back();
      _systemSegmentAllocator.release(topSegment);
      }
   }

If you've already freed _systemSegmentAllocator in TR::SystemSegmentProvider, then you will run into problems.

A child class should not be the one allocating memory for one of its parent class' members, because the parent's destructor is called after the child's.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.