Retain cycle setting delegate to valueFormatter

[edalford11]

Just migrated to 3.0. Good to go except I can't get rid of a retain cycle that seems to be coming from IChartAxisValueFormatter

Chart set up below

CGRect *frame = CGRectMake(0, 0, self.chartContainer.frame.size.width, self.chartContainer.frame.size.height);
LineChartView *chart = [[LineChartView alloc] initWithFrame:frame];
chart.legend.enabled = NO;
chart.delegate = delegate;
chart.descriptionText = @"";
chart.backgroundColor = [UIColor whiteColor];
chart.rightAxis.enabled = NO;
chart.dragEnabled = NO;
[chart setScaleEnabled:NO];
chart.pinchZoomEnabled = NO;
chart.drawGridBackgroundEnabled = NO;
chart.userInteractionEnabled = NO;
    
ChartYAxis *leftAxis = chart.leftAxis;
leftAxis.labelPosition = YAxisLabelPositionInsideChart;
leftAxis.labelFont = axisFont;
leftAxis.labelTextColor = [UIColor lightGrayColor];
leftAxis.gridColor = kDisabledGray;
    
ChartXAxis *xAxis = chart.xAxis;
xAxis.labelPosition = XAxisLabelPositionBottomInside;
xAxis.labelFont = axisFont;
xAxis.labelTextColor = [UIColor lightGrayColor];
xAxis.gridColor = kDisabledGray;        

self.mainChart.userInteractionEnabled = YES;
[self.mainChart setAccessibilityLabel:@"Landscape Price Chart"];
[self.mainChart setViewPortOffsetsWithLeft:0.0 top:5.0 right:0.0 bottom:0.0];
        
ChartYAxis *leftAxis = self.mainChart.leftAxis;
NSNumberFormatter *numberFormatter = [[NSNumberFormatter alloc] init];
[numberFormatter setNumberStyle:NSNumberFormatterDecimalStyle];
[numberFormatter setUsesGroupingSeparator:NO];
[numberFormatter setMaximumFractionDigits:2];
[numberFormatter setMinimumFractionDigits:2];
ChartDefaultAxisValueFormatter *valueFormatter = [[ChartDefaultAxisValueFormatter alloc] initWithFormatter:numberFormatter];
leftAxis.valueFormatter = valueFormatter;
        
ChartXAxis *xAxis = self.mainChart.xAxis;
xAxis.avoidFirstLastClippingEnabled = YES;
xAxis.valueFormatter = self;

BalloonMarker *marker = [[BalloonMarker alloc] initWithColor:[UIColor colorWithWhite:0/255. alpha:0.60]
                                                   font:font
                                                   textColor: [UIColor whiteColor]
                                                   insets:UIEdgeInsetsMake(8.0, 8.0, 20.0, 8.0)];
marker.minimumSize = CGSizeMake(60.f, 40.f);
self.mainChart.dragEnabled = YES;
self.mainChart.marker = marker;

LineChartDataSet *set1 = [[LineChartDataSet alloc] initWithValues:yVals label:@"Price"];
set1.lineWidth = 2.0;
set1.drawCirclesEnabled = NO;
set1.drawValuesEnabled = NO;
set1.drawFilledEnabled = YES;
set1.fillAlpha = 0.8;
set1.highlightColor = kStockTwitsBlue;

NSMutableArray *dataSets = [[NSMutableArray alloc] init];
[dataSets addObject:set1];
    
LineChartData *data = [[LineChartData alloc] initWithDataSets:dataSets];
    
self.mainChart.data = data;

Using the IChartAxisValueFormatter delegate to set the xaxis labels

- (NSString *)stringForValue:(double)value axis:(ChartAxisBase *)axis{
        return self.xLabelsIntraday[(int)value];
}

when I take out

xAxis.valueFormatter = self;

The retain cycle goes away.

I'm not familiar enough with swift to know what exactly is causing this as I generally use obj-c but it does look like there is some block passing when digging into the valueFormatter code.

[liuxuan30]

Why using self directly, shouldn't it be weakSelf? Another thing is, it's one time code; after stringForValue returns, retain count should -1? How do you find it's retain circle? Instruments report? Have you checked after you close the chart or deinit the chart, your self object can be deinit correctly? Only depending on Instruments report for retain circle sometimes is not reliable. You have to check what I asked.

[edalford11]

I tried weakSelf prior to posting this and that did not make a difference, which makes sense. After dismissing, self ends up staying in memory. I confirmed this using memory graph on xcode. If I remove that line mentioned previously, the retain cycle goes away and self deallocates as it should.

[algrid]

I'd create a separate class for value formatter instead of using self.

[liuxuan30]

@edalford11 If I understand correctly, the retain chain is:
self -> chart -> xAxis -> formatter -> self. So far it's a typical retain circle for sure. Howver, I am thinking xAxis.formatter= weakSelf should solve the retain circle. You said does not work, which needs further investigation I think.

Using another object rather than self is also workaround, but I think the point here is we figure out why weakSelf won't work. I don't think it's the library bug? Since the IAxisValueFormatter protocol contains merely a func.

[edalford11]

I don't think assigning weak properties makes a difference because those properties can still be captured strongly. I triple checked this to make sure and yes using the weakSelf pattern did not make a difference here.

I navigated to AxisBase.swift and changed

fileprivate var _axisValueFormatter: IAxisValueFormatter?

to

weak fileprivate var _axisValueFormatter: IAxisValueFormatter?

The retain cycle is now gone.

Appreciate it guys! This library is free and it does way more than most charting libraries do that are paid so keep up the good work.

[Huang-Libo]

@edalford11 @liuxuan30

There are three ways to solve this problem:

1. Use weak to modify valueFormatter (I think it's better because valueFormatter is a delegate)
2. Use weakSelf to avoid retain cycle
3. Manually set valueFormatter to nil when leave the controller.

[edalford11]

Thanks @Huang-Libo . Regarding the other issue you referenced, setting a weak property to valueModifier does not work.

The first way is the best way (Setting the delegate definition to weak) because it is known that all delegates definitions should be weak references.

Setting the delegate to weak when dismissing the view controller is bad design as these kinds of things should be cleaned up in memory automatically.

[Huang-Libo]

@edalford11
You said this works

weak fileprivate var _axisValueFormatter: IAxisValueFormatter?

[edalford11]

@Huang-Libo yes that got rid of the retain cycle for me.

[edalford11]

Scratch that. That caused some other regressions.

The problem: When wanting to implement IAxisValueFormatter via the delegate pattern it needs to be a weak reference. When it needs to be set to a custom value formatter like the DefaultAxisValueFormatter, it needs to be a strong reference.

With this approach it seems like the best way to avoid the retain cycle is to do what @algrid said and create separate class for value formatter. That being said, there is definitely a retain cycle issue when using a controller to conform to the IAxisValueFormatter protocol and that probably shouldn't be allowed in the first place with this design implementation.

[Huang-Libo]

@edalford11
However IAxisValueFormatter is author's recommended way to format axis。

[edalford11]

@Huang-Libo yes I believe its a simple design flaw that wasn't caught earlier but nothing a quick refactor can't fix. I do believe a simple class to fill in the labels with should come with the library (The way @algrid suggested). I'll submit a pull request.

Looks like 3.0.1 came out with IndexAxisValueFormatter, which is solves this problem.

[liuxuan30]

@edalford11 I am still struggling why weakSelf won't work.. As self -> chart -> xAxis -> formatter -> self shows, formatter -> weakSelf should solve the circle. Have you looked into it? Why you say it does not matter?

[edalford11]

@liuxuan30 Yes it was the first thing I tried and it did not work. And it shouldn't work. The delegate is a STRONG property. It doesn't matter if you give it a weak value, the property itself is still going to capture it strongly.

[hotuanlinh89]

@edalford11 if you use

• weak fileprivate var _axisValueFormatter: IAxisValueFormatter?
  ==> ChartYAxis will disappear .

[liuxuan30]

@hotuanlinh89 yes I agree. @edalford11 if you use weak and not using your 'self'(view controller?), the axis labels will not appear. I just tested my PR, it has issues using weak. Just like you mentioned earlier "That caused some other regressions."

Besides, I don't know why you said it's not work of using weakSelf, even if aAxis holds valueFormatter as strong, if valueFormatter is actually a weakSelf, it should work.. Just like we use weakSelf in a block, right? the valueFormatter will capture strongly but to weakSelf, not self

[Huang-Libo]

@liuxuan30
weakSelf still cause memory leak sometimes,

__weak typeof(self) weakSelf = self;
xAxis.valueFormatter = self;

self is a collectionView cell.

[liuxuan30]

@Huang-Libo your code is not valid xAxis.valueFormatter = self; why self..? a typo?
still causes memory leak "sometimes" is too obscure to say it works or not.
In your example, if your 'self' is a cell, then it's your duty to make sure it's there while formatting, or you just use another class as formatter.

[Huang-Libo]

@liuxuan30

It's a clerical error, I mean

__weak typeof(self) weakSelf = self;
xAxis.valueFormatter = weakSelf;

Maybe I need use another class as formatter.

[Huang-Libo]

@liuxuan30
@edalford11

I think we can use DefaultAxisValueFormatter( ChartDefaultAxisValueFormatter for ObjC) to avoid this kind of problem:

// An ObjC example
self.lineChartView.xAxis.valueFormatter = [ChartDefaultAxisValueFormatter withBlock:^NSString * _Nonnull(double value, ChartAxisBase * _Nullable axis) {
    // Your code here. Remember to use weakSelf in block.
}];

This code can work well in my project.

[liuxuan30]

@Huang-Libo yes using a separate class would solve that, however, towards your weakSelf plus cell, it's obscure to say 'weakSelf' work or not. In my opinion, it should work, since it's just like a block usage

[edalford11]

It has issues using weak because you designed it to be a both a delegate and a pointer to a class in memory. One causes a retain cycle and the other works. Changing it to weak solves the retain cycle but make it not work when using a class as the value. This is overall a design flaw. It really should be designed such that there is only one way to set the formatter that won't cause any retain cycles.

[liuxuan30]

I'm not sure if I am wrong in the first place, but so far no one gives a solid proof that 'weakSelf' does not work. In my opinion, weakSelf should work, just like how we use it in a block. Until we can say using weakSelf is completely different in valueFormatter than block, it's hardly a design flaw.
Using weak declaration solves the issue, but also need user to retain the delegate somewhere else, which seems a burden as well. We are used to using self as delegate in too many scenarios

[edalford11]

@liuxuan30 assigning a weak variable to a property that is defined as strong (in this case valueFormatter) does not automatically make that property weak. That is why the weak pattern does not work in this design. I have tried it myself including others in this thread. It does not work.